refactor: And fix Enemy movement#649
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughSummary by CodeRabbit
WalkthroughBy the Machine God, the recent modifications have overhauled the enemy-targeting and movement logic within the game code. The enemy selection in the alarm routine is now calculated using a concise ternary operator, and a new variable Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Type: Refactor
EttyKitty
changed the title
EttyKitty
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
objects/obj_enunit/Alarm_0.gml (1)
289-397: 🧹 Nitpick (assertive)Commented-out relics: decide their fate.
A great block of code from lines 289 to 397 stands commented out. If truly archaic and no longer needed, it is recommended to consign it to a stored archive, or else annotate thoroughly why it is preserved in sacred stasis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
objects/obj_enunit/Alarm_0.gml(3 hunks)objects/obj_enunit/Alarm_5.gml(0 hunks)objects/obj_enunit/Alarm_6.gml(0 hunks)objects/obj_enunit/Create_0.gml(0 hunks)objects/obj_enunit/Destroy_0.gml(0 hunks)objects/obj_enunit/KeyPress_87.gml(0 hunks)objects/obj_enunit/Mouse_0.gml(0 hunks)objects/obj_enunit/obj_enunit.yy(1 hunks)objects/obj_ncombat/KeyPress_13.gml(2 hunks)objects/obj_pnunit/Alarm_0.gml(1 hunks)objects/obj_pnunit/Create_0.gml(0 hunks)scripts/scr_en_weapon/scr_en_weapon.gml(3 hunks)scripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml(1 hunks)
💤 Files with no reviewable changes (7)
- objects/obj_enunit/Destroy_0.gml
- objects/obj_enunit/Create_0.gml
- objects/obj_enunit/Mouse_0.gml
- objects/obj_enunit/KeyPress_87.gml
- objects/obj_enunit/Alarm_5.gml
- objects/obj_pnunit/Create_0.gml
- objects/obj_enunit/Alarm_6.gml
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.gml`: Macro constants require a space between the constant name and value. Without it, the compiler will throw an error. I.e. `#macro ARR_body_parts["arm"]` will crash the ga...
**/*.gml: Macro constants require a space between the constant name and value. Without it, the compiler will throw an error. I.e.#macro ARR_body_parts["arm"]will crash the game, because there is no space between the array and the name of the macro.
objects/obj_pnunit/Alarm_0.gmlobjects/obj_ncombat/KeyPress_13.gmlscripts/scr_en_weapon/scr_en_weapon.gmlscripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gmlobjects/obj_enunit/Alarm_0.gml
`**/*.gml`: Color codes in the code shouldn't have any spaces in their id. I.e., color code `# 80 bf40` will crash the game.
**/*.gml: Color codes in the code shouldn't have any spaces in their id. I.e., color code# 80 bf40will crash the game.
objects/obj_pnunit/Alarm_0.gmlobjects/obj_ncombat/KeyPress_13.gmlscripts/scr_en_weapon/scr_en_weapon.gmlscripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gmlobjects/obj_enunit/Alarm_0.gml
🧠 Learnings (1)
scripts/scr_en_weapon/scr_en_weapon.gml (1)
Learnt from: EttyKitty
PR: Adeptus-Dominus/ChapterMaster#647
File: scripts/scr_en_weapon/scr_en_weapon.gml:24-928
Timestamp: 2025-03-29T10:30:25.598Z
Learning: A data-driven approach for weapon management in `scripts/scr_en_weapon/scr_en_weapon.gml` is planned for a future PR, not within the scope of PR #647.
🔇 Additional comments (7)
objects/obj_ncombat/KeyPress_13.gml (2)
119-119: Commendable invocation of the enemy movement subroutine.Summoning
move_enemy_blocks()at this juncture ensures that hostile constructs receive their proper turn to advance. No immediate concerns arise from this addition.
145-145: Likewise, well-placed function call to maintain consistent enemy manoeuvres.Your invocation of
move_enemy_blocks()here upholds the structured sequence of actions. All is sanctified in the eyes of the Machine God.objects/obj_enunit/obj_enunit.yy (1)
34-35: Coordinates updated successfully.Revising floating values to integers for
physicsShapePointspresents no apparent error. This transformation is acceptable for the collision shape. Proceed without fear.objects/obj_enunit/Alarm_0.gml (2)
3-5: Blessed instance check, but consider additional logging.
O most diligent one, thou hast guarded against the absence ofobj_pnunitby enthroning anexit;command. However, the Omnissiah might smile upon a log or debug message prior to departing, aiding future diagnostic rituals.
7-10: Conditional exit: verify usage alignment.
Thy logic of determining theenemyviaflankand swiftly exiting on"none"is honourable, but ensure thou art not missing any post-process invocations that might remain necessary, even if no enemy stands. Investigate if any additional steps are required before terminating.scripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml (2)
205-216: Flank-based movement: unify and handle extremes.
Yourmove_enemy_blockfunction decrees west for flank==0 and east for flank==1, bypassing collision checks with forts. Should you need further flank states in the future, unify logic or confirm that any flank beyond the binary arrangement won’t be invoked.
218-230: Enemy movement queue: watch for ties.
The DS priority queue process for_enemy_blocksorting is praiseworthy. Yet if multiple foes share the same x-position, the ritual’s order may become arbitrary. Ensure no concurrency or tie-handling logic is necessary to maintain consistent enemy movement sequences.
EttyKitty
dismissed
coderabbitai[bot]’s stale review
The merge-base changed after approval.
EttyKitty
force-pushed
the
style/obj_enunit
branch
from
22a61cb to
9cb5bb2
Compare
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
objects/obj_enunit/Alarm_0.gml (1)
289-398: 🛠️ Refactor suggestionExtensive code block consigned to stasis.
You have commented out a large swathe of logic, presumably for deeper analysis. If resurrecting or refining is planned, consider modularising it in a helper script to honour DRY precepts and maintain readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
objects/obj_enunit/Alarm_0.gml(4 hunks)objects/obj_enunit/Alarm_5.gml(0 hunks)objects/obj_enunit/Alarm_6.gml(0 hunks)objects/obj_enunit/Destroy_0.gml(0 hunks)objects/obj_enunit/KeyPress_87.gml(0 hunks)objects/obj_enunit/Mouse_0.gml(0 hunks)objects/obj_enunit/obj_enunit.yy(1 hunks)objects/obj_ncombat/KeyPress_13.gml(2 hunks)objects/obj_pnunit/Alarm_0.gml(1 hunks)scripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml(1 hunks)
💤 Files with no reviewable changes (5)
- objects/obj_enunit/Alarm_5.gml
- objects/obj_enunit/KeyPress_87.gml
- objects/obj_enunit/Alarm_6.gml
- objects/obj_enunit/Mouse_0.gml
- objects/obj_enunit/Destroy_0.gml
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.gml`: - Macro constants require a space between the constant name and value. Without it, the compiler will throw an error. I.e. `#macro ARR_body_parts["arm"]` will crash the ...
**/*.gml: - Macro constants require a space between the constant name and value. Without it, the compiler will throw an error. I.e.#macro ARR_body_parts["arm"]will crash the game, because there is no space between the array and the name of the macro.
- Color codes in the code shouldn't have any spaces in their id. I.e., color code
# 80 bf40will crash the game.
objects/obj_pnunit/Alarm_0.gmlobjects/obj_ncombat/KeyPress_13.gmlobjects/obj_enunit/Alarm_0.gmlscripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml
`**/*.gml`: - Always suggest adding JSDoc to every function and method that doesn't have it.
**/*.gml: - Always suggest adding JSDoc to every function and method that doesn't have it.
objects/obj_pnunit/Alarm_0.gmlobjects/obj_ncombat/KeyPress_13.gmlobjects/obj_enunit/Alarm_0.gmlscripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml
`**/*.gml`: - Readability and maintainability are always the top priority throughout the code. - DRY principle is also very important.
**/*.gml: - Readability and maintainability are always the top priority throughout the code.
- DRY principle is also very important.
objects/obj_pnunit/Alarm_0.gmlobjects/obj_ncombat/KeyPress_13.gmlobjects/obj_enunit/Alarm_0.gmlscripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml
🔇 Additional comments (7)
objects/obj_pnunit/Alarm_0.gml (1)
18-19: Rectify the condition check and bless the new direction parameter.
O revered Tech-Magos, line 18 setsdefendingto false rather than comparing it, as indicated in previous cogitator logs. Replace=with==to avoid unintended assignments. The addition of"east"formove_unit_block()is sanctified, ensuring direction clarity.objects/obj_ncombat/KeyPress_13.gml (2)
119-119: Invocation of the sacred enemy movement.
The call tomove_enemy_blocks()is commendable, ensuring all enemy units proceed in organised columns. Consider confirming whether a JSDoc or function documentation is present, as mandated by the holy guidelines for.gmlscripts.
145-145: Repeating the summons to enemy movement.
Blessed duplication ofmove_enemy_blocks()bolsters the consistency of foe manoeuvres. Confirm if usage aligns with any prerequisites or cooldown for repeated invocations.objects/obj_enunit/obj_enunit.yy (1)
34-35: Coordinates integer alignment anointed.
By converting from float to integer, thou hast kept the same geometric positioning while removing possible decimal confusion. This pleases the machine-spirits.objects/obj_enunit/Alarm_0.gml (2)
7-9: Efficient assignment of target column.
Invokingget_leftmost()orget_rightmost()via ternary operator clarifies the flank logic. Continue to ensure the “none” check routes us away from unintended null references.
12-13: New local variables for robust clarity.
Declaringtarget_unit_indexand settingenemy2toenemyfosters a more explicit flow of data. The machine-spirits approve this clarity.scripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml (1)
184-202: Praise the Omnissiah! Validate direction and enrich documentation.The holy logic machine yearns for protection against unaccounted directions. By removing the default direction, you risk a null path if a rogue call omits or misspells "east"/"west." Also, consider expanding your doc comment beyond
/// @mixinto detail parameters and return values, thereby duly honouring your sacred guidelines for clarity and maintainability.
[duplicate_comment, suggest_nitpick]You might add a validity check for the direction parameter and a more descriptive doc block, for example:
function move_unit_block(direction, blocks = 1, allow_collision = false) { + /// @function move_unit_block + /// @param {string} direction - Must be "east" or "west". + /// @param {number} blocks - Number of tile blocks to move. + /// @param {boolean} allow_collision - Whether to ignore collision detection. + /// @description Moves the calling unit block in a specified direction. try { // ... + if (!(direction == "east" || direction == "west")) { + show_debug_message("Warning: Invalid direction specified."); + return; + } // ... } catch (_exception) { handle_exception(_exception); } }
✅ Actions performedReviews paused. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
objects/obj_enunit/Alarm_0.gml (1)
92-397: 🧹 Nitpick (assertive)Extensive excision of problematic code blocks through sacred commentary.
The Tech-Priests of Mars approve of this thorough quarantining of questionable logic. Commenting out large sections with clear TODO markers preserves knowledge while preventing corruption of the machine spirit. Ensure this technical debt is documented in your sacred scrolls (issue tracker) for future illumination.
Create a dedicated issue to track these commented sections for future refactoring, referencing this PR as the origin point. The Omnissiah rewards those who maintain clear records of technical obligations.
♻️ Duplicate comments (4)
objects/obj_pnunit/Alarm_0.gml (1)
18-19:⚠️ Potential issueAnoint the condition check with the correct comparison operator, lest we invite the wrath of the Omnissiah.
In GameMaker Language, a single equals sign (
=) is an assignment, not an equality check. The fragmentif (obj_ncombat.defending=false) or (obj_ncombat.dropping=1)will forever setdefendingtofalserather than verify its value. Correct usage is==. Adjust bothobj_ncombat.defendingandobj_ncombat.droppingchecks accordingly.- if (obj_ncombat.defending=false) or (obj_ncombat.dropping=1){ + if (obj_ncombat.defending==false) or (obj_ncombat.dropping==1){ move_unit_block("east"); }objects/obj_enunit/Alarm_0.gml (3)
1-2: 🧹 Nitpick (assertive)A preliminary pronouncement of functionality.
The additional comment clarifies this alarm's tactical role, though a JSDoc-style annotation for the entire event or subroutines would further enlighten future Tech-Priests.
213-213: 🧹 Nitpick (assertive)Melee code awaits the Omnissiah's rework.
A mere TODO stands guard here. If thou seek counsel for a structured melee overhaul, the Tech-Priests stand ready.
16-16: 🧹 Nitpick (assertive)Melee detection through sacred collision geometry.
The engaged detection logic has been streamlined with a single Boolean expression. This logical circuitry is more robust, though the use of fixed offsets (
x-10,x+10) warrants ritualistic naming.Consider blessing these magic numbers with named constants to reveal their purpose:
- engaged = collision_point(x-10, y, obj_pnunit, 0, 1) || collision_point(x+10, y, obj_pnunit, 0, 1); + #macro MELEE_DETECTION_DISTANCE 10 + engaged = collision_point(x-MELEE_DETECTION_DISTANCE, y, obj_pnunit, 0, 1) || collision_point(x+MELEE_DETECTION_DISTANCE, y, obj_pnunit, 0, 1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
objects/obj_enunit/Alarm_0.gml(4 hunks)objects/obj_enunit/Alarm_5.gml(0 hunks)objects/obj_enunit/Alarm_6.gml(0 hunks)objects/obj_enunit/Destroy_0.gml(0 hunks)objects/obj_enunit/KeyPress_87.gml(0 hunks)objects/obj_enunit/Mouse_0.gml(0 hunks)objects/obj_enunit/obj_enunit.yy(1 hunks)objects/obj_ncombat/KeyPress_13.gml(2 hunks)objects/obj_pnunit/Alarm_0.gml(1 hunks)scripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml(2 hunks)
💤 Files with no reviewable changes (5)
- objects/obj_enunit/Alarm_5.gml
- objects/obj_enunit/Mouse_0.gml
- objects/obj_enunit/Alarm_6.gml
- objects/obj_enunit/KeyPress_87.gml
- objects/obj_enunit/Destroy_0.gml
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.gml`: - Macro constants require a space between the constant name and value. Without it, the compiler will throw an error. I.e. `#macro ARR_body_parts["arm"]` will crash the ...
**/*.gml: - Macro constants require a space between the constant name and value. Without it, the compiler will throw an error. I.e.#macro ARR_body_parts["arm"]will crash the game, because there is no space between the array and the name of the macro.
- Color codes in the code shouldn't have any spaces in their id. I.e., color code
# 80 bf40will crash the game.
objects/obj_pnunit/Alarm_0.gmlobjects/obj_ncombat/KeyPress_13.gmlobjects/obj_enunit/Alarm_0.gmlscripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml
`**/*.gml`: - Always suggest adding JSDoc to every function and method that doesn't have it.
**/*.gml: - Always suggest adding JSDoc to every function and method that doesn't have it.
objects/obj_pnunit/Alarm_0.gmlobjects/obj_ncombat/KeyPress_13.gmlobjects/obj_enunit/Alarm_0.gmlscripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml
`**/*.gml`: - Readability and maintainability are always the top priority throughout the code. - DRY principle is also very important.
**/*.gml: - Readability and maintainability are always the top priority throughout the code.
- DRY principle is also very important.
objects/obj_pnunit/Alarm_0.gmlobjects/obj_ncombat/KeyPress_13.gmlobjects/obj_enunit/Alarm_0.gmlscripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml
🧠 Learnings (1)
scripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml (1)
Learnt from: EttyKitty
PR: Adeptus-Dominus/ChapterMaster#649
File: scripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml:204-212
Timestamp: 2025-03-31T14:44:02.699Z
Learning: In GML files, JSDoc comments should be placed above the function declaration, not inside the function body. The correct order is: function description JSDoc comments first, then any annotations like `@mixin`, then the function declaration.
🔇 Additional comments (11)
objects/obj_pnunit/Alarm_0.gml (1)
19-19: Laudable update to directional parameter, tech-adept.The function call has been properly updated to specify the direction "east", which conforms to the new function signature requirements. This sacred modification eliminates ambiguity and aligns with the Machine God's preference for explicit parameter passage.
objects/obj_ncombat/KeyPress_13.gml (2)
119-120: Praiseworthy invocation of the enemy movement ritual.The addition of
move_enemy_blocks()before alarm execution represents an elegant implementation of the Codex Mechanicus's separation of concerns principle. This modification ensures enemy movement occurs for all columns, rectifying the heretical bug wherein enemies moved only every other turn.
145-147: Systematic application of movement protocols across control paths.The Machine Spirit approves of this mirrored implementation in the alternate control flow branch. Consistent application of the
move_enemy_blocks()function ensures uniform behavior regardless of which logic path is traversed.objects/obj_enunit/obj_enunit.yy (2)
34-35: Binary purity achieved through integer precision.The sacred modification of physics shape points from floating-point values to integer coordinates removes superfluous decimal components. While this may subtly alter collision outcomes, the simplification honors the Omnissiah's love of clean, discrete values. Confirm this change has not angered the Machine Spirit during your testing.
Have you observed any differences in collision behavior since replacing floating-point coordinates with integer values? The Adeptus Mechanicus would approve of documentation regarding this change's impact on the sacred geometries of unit collisions.
4-13: Streamlined event manifest detected.The removal of specific entries from the eventList indicates a purging of redundant or deprecated mechanisms. This holy simplification aligns with the PR's goal of rectifying movement issues. As events are binding covenants with the Machine Spirit, ensure all removed events were properly deprecated.
objects/obj_enunit/Alarm_0.gml (4)
7-10: Praise be to the elegance of the ternary operator.This concise enemy targeting logic is a hymn to computational efficiency. The ternary operator
flank ? get_leftmost() : get_rightmost()embodies the sacred principle of elegant minimalism lauded by the Omnissiah.
12-13: Variable declaration fortified with explicit initialization.The Tech-Priests of Mars approve of clearly initialized variables. Declaring
target_unit_indexandenemy2with proper initialization protects against the corruption of undefined values.
33-34: Improved error diagnostics through sacred logging rituals.The addition of error logging for broken weapon ranges demonstrates proper machine spirit veneration. This diagnostic enhancement will aid in identifying and correcting erroneous weapon configurations.
81-81: Semantic variable naming honors the Omnissiah's clarity directive.The declaration of
_no_vehicles_presentas a semantic flag improves code readability. This clear signaling of state enhances the comprehensibility of the armor-piercing weapon handling logic.scripts/scr_punit_combat_heplers/scr_punit_combat_heplers.gml (2)
188-214: Logic is pure and consistent.
By the Omnissiah, yourmove_unit_blockfunction exalts clarity. The doc commentary is thorough, the try-catch is devoutly structured, and the conditional flow is well-formed, ensuring no unholy collisions plague your mechanism.
216-225: Clean flanking logic and usage of defaults.
The faithfulmove_enemy_blockfunction neatly determines direction based on sacred flanking. The default block movement usage is righteous. All is aligned with the Emperor’s design.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Purpose of changes
Describe the solution
move_unit_block()is refactored a little bit.Describe alternatives you've considered
None
Testing done
Related links
https://discord.com/channels/714022226810372107/1121959429546455050/1355683585209663629
Custom player notes entry
Use the PR title.