Слизнекоты. #3281
Open
nopeingeneer wants to merge 9 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 3 minutes and 24 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughДобавлен новый файл ChangesSlugcat friendly pet mob
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Слизнекотики |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueИспользуйте grant_action вместо устаревшего add_verb.
Паттерн
add_verb()устарел. Для добавления действий (verbs) предпочтительно использовать систему actions черезgrant_action().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm` at line 45, The add_verb() call in the slugcat initialization is using a deprecated pattern for adding verbs to mobs. Replace the add_verb(src, /mob/living/proc/lay_down) call with the grant_action() function instead, which is the preferred modern approach for adding actions to mobs. Ensure you pass the appropriate action object or reference to grant_action() that corresponds to the lay_down action you want to grant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm`:
- Around line 216-220: The show_inv() method displays UI elements for collar
interactions with href links for remove_inv=collar and add_inv=collar actions,
but the Topic() method does not have any code to handle these actions. Add
handling in the Topic() method for both remove_inv=collar and add_inv=collar
cases similar to how other inventory actions are handled in that method (check
for these parameters in the href and implement the corresponding logic).
Alternatively, if collar functionality is not intended to be implemented, remove
the collar-related href lines from the show_inv() method to avoid showing UI
elements that do nothing when clicked.
- Around line 84-86: The slugcat mob has initialization logic in the New()
method that conflicts with the existing Initialize() method. Move the
regenerate_icons() call from the New() method to the end of the existing
Initialize() method, then delete the entire New() method. This ensures all atom
initialization follows the proper pattern by using Initialize() instead of
New().
- Around line 107-110: The logical condition in the if statement at slugcat.dm
uses incorrect negation that reverses the intended access control logic. The
condition `!(iscarbon(usr) || usr.incapacitated() || !Adjacent(usr))` currently
denies access only to non-carbon mobs that are capable and adjacent, while
allowing carbon mobs unrestricted access. Remove the outer negation operator
from the condition so that it reads without the leading exclamation mark, which
will correctly close the browser window when the user is not carbon, is
incapacitated, or is not adjacent to the slugcat.
---
Nitpick comments:
In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm`:
- Line 45: The add_verb() call in the slugcat initialization is using a
deprecated pattern for adding verbs to mobs. Replace the add_verb(src,
/mob/living/proc/lay_down) call with the grant_action() function instead, which
is the preferred modern approach for adding actions to mobs. Ensure you pass the
appropriate action object or reference to grant_action() that corresponds to the
lay_down action you want to grant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c677bac4-d63b-4b4a-9329-bb76691db1d3
📒 Files selected for processing (3)
modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dmmodular_sand/icons/mob/animal.dmitgstation.dme
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm (5)
28-29: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winДобавьте
Destroy()для очистки ссылок на предметы.
inventory_headиinventory_handдержат сильные ссылки на datums/items, но тип не очищает их при qdel slugcat'а внеdeath(). Это оставляет риск зависших contents/ref cycles при удалении моба напрямую.🧹 Возможное исправление
+/mob/living/simple_animal/pet/slugcat/Destroy() + if(inventory_head) + inventory_head.forceMove(get_turf(src)) + inventory_head.dropped(src) + null_hat() + if(inventory_hand) + inventory_hand.forceMove(get_turf(src)) + inventory_hand.dropped(src) + null_hand() + return ..() + /mob/living/simple_animal/pet/slugcat/proc/null_hand() unspeared() inventory_hand = nullAs per path instructions: «Destroy() must call parent and return a QDEL_HINT» и «Destroy() must null out all references to other datums to allow GC».
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm` around lines 28 - 29, The slugcat type declares strong references to items via the variables inventory_head and inventory_hand, but does not clean up these references when the object is deleted outside the death() function, creating potential garbage collection issues and reference cycles. Implement a Destroy() method in the slugcat type that nulls out both inventory_head and inventory_hand references to break any lingering references to datum objects, then call the parent Destroy() method and return an appropriate QDEL_HINT to allow proper garbage collection.Source: Path instructions
158-170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winСбрасывайте offset шляпы независимо от наличия шляпы.
Если slugcat лёг, затем шляпу сняли, а потом он встал,
hat_offset_yостанетсяhat_offset_y_rest, потому что строка 167 сбрасывает offset только приinventory_head. Следующая надетая шляпа будет отрисована как для лежачего состояния.🐛 Предлагаемое исправление
/mob/living/simple_animal/pet/slugcat/update_mobility() . = ..() if(stat != DEAD) if(!CHECK_MOBILITY(src, MOBILITY_STAND)) - if(inventory_head || inventory_hand) - hat_offset_y = hat_offset_y_rest + hat_offset_y = hat_offset_y_rest + if(inventory_hand) drop_hand() icon_state = "[icon_living]_rest" else - if(inventory_head) - hat_offset_y = initial(hat_offset_y) + hat_offset_y = initial(hat_offset_y) icon_state = "[icon_living]" regenerate_icons()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm` around lines 158 - 170, In the update_mobility() function for the slugcat pet, the hat offset is only being reset to its initial value when the creature is standing AND has a hat in inventory. However, the offset should be reset whenever the creature stands up, regardless of whether a hat is currently equipped. Move the hat_offset_y assignment that sets it to initial(hat_offset_y) outside of the inventory_head conditional check so that it executes for all standing creatures, not just those with a hat equipped.
210-220: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winЛокализуйте UI labels в popup.
Nothing,Inventory of,HeadиHand— пользовательские labels в browser UI, а не технические identifiers. Сейчас они выбиваются из русскоязычных сообщений этого же интерфейса.🌐 Предлагаемое исправление
- head_href = "<A href='?src=\[UID()\];add_inv=head'>Nothing</A>" + head_href = "<A href='?src=\[UID()\];add_inv=head'>Пусто</A>" @@ - hand_href = "<A href='?src=\[UID()\];add_inv=hand'>Nothing</A>" + hand_href = "<A href='?src=\[UID()\];add_inv=hand'>Пусто</A>" @@ - var/dat = {"<meta charset="UTF-8"><div align='center'><b>Inventory of \[name\]</b></div><p>"} - dat += "<br><B>Head:</B> [head_href]" - dat += "<br><B>Hand:</B> [hand_href]" + var/dat = {"<meta charset="UTF-8"><div align='center'><b>Инвентарь \[name\]</b></div><p>"} + dat += "<br><B>Голова:</B> [head_href]" + dat += "<br><B>Лапы:</B> [hand_href]"As per path instructions: English is fine for
name/descand mechanic identifiers, but user-facing chat/UI labels should be localized.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm` around lines 210 - 220, The UI labels displayed in the browser popup (Nothing, Inventory of, Head, and Hand) are hardcoded in English and should be localized to match the interface language. Replace the hardcoded English strings in the head_href and hand_href variable assignments and in the dat string construction with appropriate localization function calls. Specifically, update the "Nothing" text in both href assignments, the "Inventory of" text in the meta charset section, and the "Head:" and "Hand:" labels in the dat concatenation lines to use your localization system instead of hardcoded English strings.Source: Path instructions
298-304: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winПроверяйте
is_pacifistдо сброса предмета из рук.Для monk/pacifist slugcat строка 298 уже роняет копьё на землю, а строки 302-304 потом отказывают в экипировке. В итоге отклонённое действие всё равно меняет инвентарь пользователя.
🐛 Предлагаемое исправление
- if(user && !user.dropItemToGround(item_to_add)) - to_chat(user, span_warning("Предмет застрял в ваших руках!")) - return FALSE - if(is_pacifist) to_chat(user, span_warning("Этот слизнекот - пацифист и не пользуется оружием!")) return FALSE + + if(user && !user.dropItemToGround(item_to_add)) + to_chat(user, span_warning("Предмет застрял в ваших руках!")) + return FALSE🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm` around lines 298 - 304, The `is_pacifist` check is performed after the `dropItemToGround` operation, which means a pacifist slugcat will still drop the item even though the action is subsequently rejected. Move the `is_pacifist` condition check before the user.dropItemToGround call so that the function returns FALSE without modifying the user's inventory when the slugcat is a pacifist. This ensures rejected actions do not have side effects on inventory state.
206-221: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winЭкранируйте динамические значения перед вставкой в browser HTML.
name,[inventory_head],[inventory_hand]и[src]попадают в HTML безhtml_encode(). Если имя предмета/моба содержит HTML, popup можно сломать или внедрить лишние ссылки/разметку.🛡️ Предлагаемое исправление
var/head_href if(inventory_head) - head_href = "<A href='?src=\[UID()\];remove_inv=head'>\[inventory_head\]</A>" + var/head_name = html_encode("[inventory_head]") + head_href = "<A href='?src=\[UID()\];remove_inv=head'>[head_name]</A>" else head_href = "<A href='?src=\[UID()\];add_inv=head'>Nothing</A>" var/hand_href if(inventory_hand) - hand_href = "<A href='?src=\[UID()\];remove_inv=hand'>\[inventory_hand\]</A>" + var/hand_name = html_encode("[inventory_hand]") + hand_href = "<A href='?src=\[UID()\];remove_inv=hand'>[hand_name]</A>" else hand_href = "<A href='?src=\[UID()\];add_inv=hand'>Nothing</A>" - var/dat = {"<meta charset="UTF-8"><div align='center'><b>Inventory of \[name\]</b></div><p>"} + var/slugcat_name = html_encode("[name]") + var/dat = {"<meta charset="UTF-8"><div align='center'><b>Inventory of [slugcat_name]</b></div><p>"} dat += "<br><B>Head:</B> [head_href]" dat += "<br><B>Hand:</B> [hand_href]" - var/datum/browser/popup = new(user, "mob\[UID()\]", "\[src\]", 440, 250) + var/datum/browser/popup = new(user, "mob\[UID()\]", html_encode("[src]"), 440, 250)Based on learnings:
html_encode()безопасен для Unicode/Cyrillic и не ломает русские строки.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm` around lines 206 - 221, The code constructs HTML content with dynamic values from name, inventory_head, inventory_hand, and src without properly escaping them using html_encode(). This allows potentially malicious or malformed HTML in these values to break the popup layout or inject unwanted markup. Wrap all dynamic values being inserted into the dat HTML string with html_encode() to safely escape any HTML special characters. Specifically, apply html_encode() to the name variable when inserted into the dat string, to inventory_head and inventory_hand when used as text content in the anchor tags, and to src when inserted as the window title in the browser datum creation.Source: Learnings
🧹 Nitpick comments (1)
modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm (1)
32-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winВынесите неочевидные числовые значения в
#define.Смещения шляпы и делитель урона копья являются gameplay/icon tuning constants; сейчас их смысл теряется в
-8,-19и2.♻️ Предлагаемое исправление
+#define SLUGCAT_HAT_OFFSET_Y -8 +#define SLUGCAT_HAT_OFFSET_Y_REST -19 +#define SLUGCAT_REDUCED_SPEAR_DAMAGE_DIVISOR 2 + /mob/living/simple_animal/pet/slugcat @@ - var/hat_offset_y = -8 - var/hat_offset_y_rest = -19 + var/hat_offset_y = SLUGCAT_HAT_OFFSET_Y + var/hat_offset_y_rest = SLUGCAT_HAT_OFFSET_Y_REST @@ - melee_damage_lower = round(spear_weapon.force / (is_reduce_damage ? 2 : 1)) - melee_damage_upper = round(spear_weapon.force / (is_reduce_damage ? 2 : 1)) + melee_damage_lower = round(spear_weapon.force / (is_reduce_damage ? SLUGCAT_REDUCED_SPEAR_DAMAGE_DIVISOR : 1)) + melee_damage_upper = round(spear_weapon.force / (is_reduce_damage ? SLUGCAT_REDUCED_SPEAR_DAMAGE_DIVISOR : 1))As per path instructions: «No magic numbers — use
#defineconstants».Also applies to: 179-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm` around lines 32 - 33, The hardcoded numeric values -8, -19, and 2 used for hat offset adjustments and spear damage calculations are magic numbers that reduce code clarity. Create `#define` constants with descriptive names for each of these values (the hat_offset_y value of -8, the hat_offset_y_rest value of -19, and the spear damage divisor of 2) at the top of the file, then replace all instances of these magic numbers throughout the slugcat.dm file with references to the corresponding `#define` constants to make the gameplay tuning values self-documenting.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm`:
- Around line 28-29: The slugcat type declares strong references to items via
the variables inventory_head and inventory_hand, but does not clean up these
references when the object is deleted outside the death() function, creating
potential garbage collection issues and reference cycles. Implement a Destroy()
method in the slugcat type that nulls out both inventory_head and inventory_hand
references to break any lingering references to datum objects, then call the
parent Destroy() method and return an appropriate QDEL_HINT to allow proper
garbage collection.
- Around line 158-170: In the update_mobility() function for the slugcat pet,
the hat offset is only being reset to its initial value when the creature is
standing AND has a hat in inventory. However, the offset should be reset
whenever the creature stands up, regardless of whether a hat is currently
equipped. Move the hat_offset_y assignment that sets it to initial(hat_offset_y)
outside of the inventory_head conditional check so that it executes for all
standing creatures, not just those with a hat equipped.
- Around line 210-220: The UI labels displayed in the browser popup (Nothing,
Inventory of, Head, and Hand) are hardcoded in English and should be localized
to match the interface language. Replace the hardcoded English strings in the
head_href and hand_href variable assignments and in the dat string construction
with appropriate localization function calls. Specifically, update the "Nothing"
text in both href assignments, the "Inventory of" text in the meta charset
section, and the "Head:" and "Hand:" labels in the dat concatenation lines to
use your localization system instead of hardcoded English strings.
- Around line 298-304: The `is_pacifist` check is performed after the
`dropItemToGround` operation, which means a pacifist slugcat will still drop the
item even though the action is subsequently rejected. Move the `is_pacifist`
condition check before the user.dropItemToGround call so that the function
returns FALSE without modifying the user's inventory when the slugcat is a
pacifist. This ensures rejected actions do not have side effects on inventory
state.
- Around line 206-221: The code constructs HTML content with dynamic values from
name, inventory_head, inventory_hand, and src without properly escaping them
using html_encode(). This allows potentially malicious or malformed HTML in
these values to break the popup layout or inject unwanted markup. Wrap all
dynamic values being inserted into the dat HTML string with html_encode() to
safely escape any HTML special characters. Specifically, apply html_encode() to
the name variable when inserted into the dat string, to inventory_head and
inventory_hand when used as text content in the anchor tags, and to src when
inserted as the window title in the browser datum creation.
---
Nitpick comments:
In `@modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm`:
- Around line 32-33: The hardcoded numeric values -8, -19, and 2 used for hat
offset adjustments and spear damage calculations are magic numbers that reduce
code clarity. Create `#define` constants with descriptive names for each of these
values (the hat_offset_y value of -8, the hat_offset_y_rest value of -19, and
the spear damage divisor of 2) at the top of the file, then replace all
instances of these magic numbers throughout the slugcat.dm file with references
to the corresponding `#define` constants to make the gameplay tuning values
self-documenting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad110c46-181a-438b-835a-c3397d337970
📒 Files selected for processing (1)
modular_bluemoon/code/modules/living/simple_animal/friendly/slugcat.dm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Переношу слизнекотов с 1984. ПР не доделан - не мерджить.
Summary by CodeRabbit
Обновления