rebalance: антитела вирусу лояльности #8473
rebalance: антитела вирусу лояльности #8473SCPfollower wants to merge 15 commits intoss220-space:master220from
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Аналитический отчёт от Большого Брата. Обработка запроса завершена.
Приветствую! Я проанализировал предложенные Вами изменения. С технической точки зрения, работа выглядит удовлетворительно.
Сводка анализа:
- 🔴 Критических замечаний: 0
- 🟡 Рекомендаций: 2
- 💡 Предложений: 0
🟡 Рекомендации по улучшению:
code/datums/diseases/viruses/_virus.dm:104: Комментарий к глобальному спискуno_vaccine_virusesописывает что делает код, а не почему. Рекомендуется добавить пояснение о причине такого решения (например, для игрового баланса), как того требует стайлгайд.code/modules/reagents/chemistry/machinery/pandemic.dm:317: Обнаружена потенциально нестабильная операция: изменение спискаresво время итерации по нему. Это может привести к пропуску элементов. Рекомендуется итерироваться по копии списка (res.Copy()) для избежания ошибок.
Пожалуйста, ознакомьтесь с комментариями к отдельным строкам кода для получения более подробной информации.
Спасибо за ваш вклад в проект!
|
|
||
| if(Blood.data["resistances"]) | ||
| var/list/res = Blood.data["resistances"] | ||
| for(var/ignore_resistance in res) |
There was a problem hiding this comment.
🟡 Рекомендация по стабильности: Вы изменяете список res во время итерации по нему. В DM это может привести к пропуску элементов или другим непредсказуемым результатам. Для безопасного удаления элементов из списка во время итерации следует итерироваться по его копии.
for(var/ignore_resistance in res.Copy())
| spread += "Воздушно-капельный" | ||
| return russian_list(spread, "Неизвестено", " и ") | ||
|
|
||
| //List of viruses that cannot be vaccinated against using PANDEMIC |
There was a problem hiding this comment.
🟡 Рекомендация: Комментарий объясняет что делает код, а не почему это необходимо. Согласно стайлгайду (п. 4.1, правило 68), комментарии должны объяснять причину, а не повторять код. В данном случае, это изменение баланса, и комментарий должен это отражать.
// For game balance, some viruses are not intended to be preventable with a simple vaccine.
References
- Комментарии должны объяснять 'почему' код написан так, а не 'что' он делает. Комментарии должны быть на английском. (link)
|
Весь нейрослоп буду тыкать завтра |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adjusts the vaccine creation logic in the Pandemic machine, including allowing antibody development for loyalty syndrome, adding ether to medicines, and filtering non-vaccinable viruses. However, it introduces a significant security vulnerability in the vaccine creation process. The change from an index-based lookup to a direct path-based request from user input, without proper validation, allows for arbitrary object instantiation and bypasses the requirement for a valid blood sample to create vaccines. Furthermore, the current vaccine creation logic needs refinement to improve code stability and readability, especially concerning potential errors when interacting with the disease archive.
| var/vaccine_type = text2path(href_list["create_vaccine"]) | ||
|
|
||
| if(vaccine_type in GLOB.no_vaccine_viruses) | ||
| to_chat(usr, span_warning("К этому вирусу нельзя создать вакцину.")) | ||
| return | ||
|
|
||
| var/vaccine_name = UNKNOWN_STATUS_RUS | ||
|
|
||
| if(!ispath(vaccine_type)) | ||
| if(GLOB.archive_diseases[path]) | ||
| var/datum/disease/disease = GLOB.archive_diseases[path] | ||
| if(GLOB.archive_diseases[vaccine_type]) | ||
| var/datum/disease/disease = GLOB.archive_diseases[vaccine_type] | ||
| if(disease) | ||
| vaccine_name = disease.name | ||
| vaccine_type = path | ||
| vaccine_type = vaccine_type | ||
| else if(vaccine_type) | ||
| var/datum/disease/disease = new vaccine_type |
There was a problem hiding this comment.
This section introduces a critical security vulnerability. The use of text2path on raw user input from href_list["create_vaccine"] followed by instantiation with new allows for arbitrary object instantiation. An attacker can craft a request with a path to any object (e.g., /obj/item/gun/energy/laser), bypassing the machine's intended logic and the requirement for a valid blood sample to create vaccines.
Beyond the security concern, the logic for accessing the disease archive (GLOB.archive_diseases) and subsequent checks are incorrect and redundant. After href_list["create_vaccine"] is changed to pass a string path type, vaccine_type will either be a valid type-path or null. The current if(!ispath(vaccine_type)) check only triggers if vaccine_type is null, which would cause an error when GLOB.archive_diseases[vaccine_type] attempts to use null as a key. Additionally, GLOB.archive_diseases is likely indexed by string identifiers (e.g., disease.GetDiseaseID()), not type-paths directly, requiring vaccine_type.type for correct access.
To remediate the security vulnerability, you should:
- Verify that the requested
vaccine_typeis a subtype of/datum/diseaseusingispath(vaccine_type, /datum/disease). - Ensure that the requested
vaccine_typeis actually present in the resistances list of the blood sample currently in the machine's beaker.
The provided code suggestion addresses the logical issues by simplifying checks and correctly using the string representation of the type-path for GLOB.archive_diseases access.
var/vaccine_name = UNKNOWN_STATUS_RUS
var/datum/disease/disease_instance
if(!vaccine_type) // Если text2path вернул null, значит, это невалидный путь
to_chat(usr, span_warning("Неверный тип вакцины."))
return
// Попытка найти болезнь в архиве по строковому представлению пути
if(GLOB.archive_diseases[vaccine_type.type])
disease_instance = GLOB.archive_diseases[vaccine_type.type]
if(disease_instance)
vaccine_name = disease_instance.name
else // Если не найдено в архиве, создаем новый экземпляр из тип-пути
disease_instance = new vaccine_type
if(disease_instance)
vaccine_name = disease_instance.name
SCPfollower
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
Аналитический отчёт от Большого Брата. Обработка запроса завершена.
Приветствую! Я проанализировал предложенные Вами изменения. С технической точки зрения, работа выглядит удовлетворительно.
Сводка анализа:
- 🔴 Критических замечаний: 0
- 🟡 Рекомендаций: 1
- 💡 Предложений: 0
🟡 Рекомендации по улучшению:
code/modules/reagents/chemistry/machinery/pandemic.dm:364-368: Использование цикла для фильтрации списка можно заменить на более эффективную и лаконичную операцию вычитания списков.
Пожалуйста, ознакомьтесь с комментариями к отдельным строкам кода для получения более подробной информации.
Спасибо за ваш вклад в проект!
|
там же в дефайне самого вируса всего в одной строке надо же было поставить тру вместо фолс... Или я недочитал все коды? |
|
Идея следующая: если выпить лекарства, то у человека будут антитела и он не сможет заболеть вновь, НО вакцину к лояльности не сделать. Проблема в том, что ранее такой прикол не был реализован, так что пришлось люто костылить (или переписывать весь код пандемика, но я не могу и не хочу) |
|
Сейчас или к болезни нельзя выработать антитела вообще, или можно и к ней делается вакцина за 30 секунд после этого |
|
Короче как и писал в обсуждении идей, хочу сесть на два стула, где вирус поддается лечению, но при этом не будет "ВАКЦИНА НА СТОЙКЕ МЕДА" через 5 минут после появления вируса |
|
/gemini review |
There was a problem hiding this comment.
Code Review
Аналитический отчёт от Большого Брата. Обработка запроса завершена.
Приветствую! Я проанализировал предложенные Вами изменения. С технической точки зрения, работа выглядит удовлетворительно.
Сводка анализа:
- 🔴 Критических замечаний: 0
- 🟡 Рекомендаций: 1
- 💡 Предложений: 0
🟡 Рекомендации по улучшению:
code/modules/reagents/chemistry/machinery/pandemic.dm:362: Реализация проверки реагентов может быть улучшена для большей надёжности. Рекомендуется добавить явную проверку на тип реагента (кровь), чтобы избежать потенциальных конфликтов с другими реагентами в будущем.
Пожалуйста, ознакомьтесь с комментариями к отдельным строкам кода для получения более подробной информации.
Спасибо за ваш вклад в проект!
тогда круто |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to rebalance the loyalty syndrome virus, making its treatment more complex by allowing for antibodies, and introduces a mechanism to the Pandemic machine to prevent vaccine creation for specific viruses. However, a critical security vulnerability has been identified: the implementation uses an in-place list modification on a potentially shared reference, which can lead to unintended side effects, such as removing immunity from the source mob. Additionally, a potential null object access was found in code/modules/reagents/chemistry/machinery/pandemic.dm that could cause runtime errors.
SCPfollower
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
Аналитический отчёт от Большого Брата. Обработка запроса завершена.
Приветствую! Я проанализировал предложенные Вами изменения. С технической точки зрения, работа выглядит удовлетворительно.
Изменения вносят ожидаемый ребаланс вируса лояльности, добавляя возможность выработки иммунитета и усложняя его лечение. Также корректно реализован механизм запрета на создание вакцин для определённых вирусов.
Сводка анализа:
- 🔴 Критических замечаний: 0
- 🟡 Рекомендаций: 1
- 💡 Предложений: 0
🟡 Рекомендации по улучшению:
code/modules/reagents/chemistry/machinery/pandemic.dm:367: Обнаружено создание нового объекта в цикле только для проверки свойства. Рекомендуется использоватьinitial()для повышения производительности и избежания ненужной нагрузки на сборщик мусора.
Пожалуйста, ознакомьтесь с комментариями к отдельным строкам кода для получения более подробной информации.
Спасибо за ваш вклад в проект!
| var/datum/disease/virus/virus_res = new path | ||
| if(virus_res.no_vaccine) |
There was a problem hiding this comment.
🟡 Рекомендация по производительности
Создание нового объекта (new path) внутри цикла для проверки одного свойства (no_vaccine) является избыточным и может негативно сказаться на производительности, особенно если в мензурке много реагентов с большим количеством антител. Создание лишних объектов без необходимости также увеличивает нагрузку на сборщик мусора.
Согласно разделу 4.2 стайлгайда, следует избегать ресурсоемких операций в циклах. Для получения значения переменной типа по умолчанию рекомендуется использовать initial(), что является более эффективным и безопасным подходом.
if(initial(path.no_vaccine))
References
- Стиль-гайд рекомендует избегать ресурсоемких операций в циклах для поддержания производительности и стабильности. Создание новых объектов в цикле без необходимости является такой операцией. (link)
There was a problem hiding this comment.
Не сработало(
В целом у нас в теоретическом максимуме может быть 10 разных антител, а new вызывается только при установке контейнера в Пандемик. Так что наверное можно оставить так
3 participants
Что этот ПР делает
Добавляет возможность выработать антитела к вирусу лояльности; усложняет его лечение необходимостью использовать помимо галоперидола еще и эфир; добавляет список вирусов, к которым нельзя сделать вакцину в Пандемике (no_vaccine_viruses, там пока только лояльность)
Почему это хорошо для игры
Самый бесящий вирус теперь можно нормально вылечить и не бояться каждого прохожего, что может вновь заразить тебя. Однако вирус не убит полностью и все еще представляет угрозу
Демонстрация изменений
Демонстрации изменений
Тестирование
Проверял на локалке