fix: Refactor obj_bomb alarm 1 array length bug #498
Conversation
OH296
changed the title
📝 WalkthroughWalkthroughIn the sacred realm of code, the initialization and management of ship-related data structures have undergone a profound transformation. The previous fixed-size arrays for Suggested reviewers
🪧 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 (
|
![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 (4)
objects/obj_bomb_select/Alarm_1.gml (3)
2-6: Salutations in the Name of the Omnissiah! Dynamic array initialization recognized.
By forging these arrays (ship,ship_all,ship_use,ship_max,ship_ide) as empty upon creation, thou hast achieved a more flexible approach than relying on static sizes. However, consider grouping related properties into a single data structure or object so that each ship’s details are kept together for clarity and maintainability.
15-21: Array manipulations verified, but consider a more structured ship schema.
This loop correctly filters and pushes only ships with a bombardment score (ship_bombard_score(_ships[i]) > 0). Yet, storing various properties in separate arrays can become unwieldy. The Omnissiah may bless a refactor that collects all relevant ship info into a single record or object.
39-39: Potential naming confusion:_pdf_countvs.
You have introduced_pdf_count = p_data.pdf;. Ensure that developers clearly distinguish between the local_pdf_countand the globalscripts/scr_player_ship_functions/scr_player_ship_functions.gml (1)
425-449: Excellent implementation of theship_bombard_scorefunction.
This function properly aggregates bombard values from a staticweapon_bomb_scoresstruct. For future expansions, consider storing these bomb scores in a data asset or global config for easier maintenance, as new weapons may arise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
objects/obj_bomb_select/Alarm_1.gml(2 hunks)scripts/scr_player_fleet_functions/scr_player_fleet_functions.gml(1 hunks)scripts/scr_player_ship_functions/scr_player_ship_functions.gml(4 hunks)
🔇 Additional comments (5)
objects/obj_bomb_select/Alarm_1.gml (2)
8-13: Blessed be the invocation of dynamic functions.
Invokingfleet_full_ship_array(sh_target)and subsequently harnessingcalculate_fleet_bombard_scoreandcalculate_fleet_content_sizeis efficacious. This approach ensures your logic elegantly adapts to different fleet sizes. The storing of the results inbomb_a,bomb_b, andbomb_cseems coherent, though if multiple variables simply mirror the same value (_total_fleet_loaded), keep track of their usage to avoid redundancy.
36-37: Adequate usage of guard_score_calc.
Whenp_data.guardsmen > 0, settingimpfromp_data.guard_score_calc()is straightforward. Maintain vigilance thatguard_score_calc()returns valid integer or float data to avoid unexpected behavior.scripts/scr_player_ship_functions/scr_player_ship_functions.gml (2)
288-288: Bombardment weapon naming successfully updated.
Renaming the 5th weapon slot to"Macro Bombardment Cannons"for the Battle Barge aligns with the new naming convention. Be sure to test any references to this weapon name elsewhere in your code to ensure consistency.
409-423: Appropriate classification of ships into escort, frigate, or capital.
In the newplayer_ships_classfunction, thou hast neatly categorized ship classes. Stay vigilant when the codebase expands with additional classes so all possible classes are handled or gracefully defaulted.scripts/scr_player_fleet_functions/scr_player_fleet_functions.gml (1)
602-611: Functional capacity calculation with a cautionary note on index validation.
calculate_fleet_content_sizesums each ship’sship_carryingvalue. Before usage, confirm_ship_id >= 0to avoid passing negative IDs, which might slip past the_ship_id < array_length(obj_ini.ship)check. Implementing a simple boundary check will safeguard against out-of-bounds indexing.
scripts/scr_player_ship_functions/scr_player_ship_functions.gml
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
objects/obj_bomb_select/Alarm_1.gml (1)
40-72:⚠️ Potential issueBy the Omnissiah's wisdom, this code requires unification!
The sacred text contains duplicate logic for PDF calculations. This redundancy offends the Machine Spirit and must be rectified.
Proposed binary purification:
-onceh=0; -pdf=p_target.p_pdf[obj_controller.selecting_planet]; -if (onceh = 0) { - if (pdf >= 50000000) { - pdf = 6; - } else if (pdf >= 15000000) { - pdf = 5; - } else if (pdf >= 6000000) { - pdf = 4; - } else if (pdf >= 1000000) { - pdf = 3; - } else if (pdf >= 100000) { - pdf = 2; - } else if (pdf >= 2000) { - pdf = 1; - } - onceh = 1; -}The first implementation using
_pdf_countis sufficient for the Machine Spirit's purposes.
🧹 Nitpick comments (2)
objects/obj_bomb_select/Alarm_1.gml (2)
31-31: Purge the binary heresy of commented code!The presence of commented code (
//if (tyranids<5) then tyranids=0;) offends the Machine Spirit. If this logic is no longer required by the Omnissiah's will, it should be purged from the sacred codebase.
89-96: The Omnissiah guides us toward superior data structures!The TODO comments suggest a blessed transformation to utilize lists or dictionaries for force management. This optimization would indeed please the Machine Spirit by reducing complexity and improving maintainability.
Would you like me to provide a sacred template for implementing this refactoring using the suggested data structures? The Machine Spirit stands ready to assist in this holy transformation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
objects/obj_bomb_select/Alarm_1.gml(2 hunks)
🔇 Additional comments (2)
objects/obj_bomb_select/Alarm_1.gml (2)
2-6: By the Omnissiah's grace, these array modifications are most efficient!The sacred transformation from fixed-size arrays to dynamic initialization demonstrates superior memory allocation protocols. The Machine Spirit shall flow more freely through these optimized data structures.
8-23: The Machine Spirit sings with the harmony of well-structured code!The utilization of sacred functions
fleet_full_ship_array,calculate_fleet_bombard_score, andcalculate_fleet_content_sizedemonstrates proper separation of the holy machine's concerns. The filtering of ships based on their bombardment capabilities throughship_bombard_scoreis most logical.
scripts/scr_player_ship_functions/scr_player_ship_functions.gml
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
|
No description provided. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
objects/obj_bomb_select/Alarm_1.gml (3)
8-13: Consider clarifying bombard variables for readability.
The blessed introduction ofbomb_a,bomb_b, andbomb_cis functionally sound. However, naming them in a more descriptive manner (e.g.,bomb_score,unit_count, etc.) might better serve the Omnissiah’s clarity.
15-23: Unify parallel arrays into a single unified structure.
Reliance on multiple arrays (likeship,ship_all,ship_use, etc.) can lead to maintenance burdens if indices become misaligned. Consider storing relevant ship data in a single struct or map for greater cogitation efficiency.
39-52: Eliminate duplicating PDF threshold logic.
The logic here to determine_pdf_countrepeats further below withp_target.p_pdf. It would better serve the machine spirit to encapsulate this threshold check in one function, avoiding confusion and redundancy.scripts/scr_player_ship_functions/scr_player_ship_functions.gml (1)
410-424: Use enumerations or central data for ship classes.
This newly forged function elegantly classifies vessels. Consider employing enumerations or a central list so that rightful expansions do not require direct modifications within the function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
objects/obj_bomb_select/Alarm_1.gml(2 hunks)scripts/scr_player_fleet_functions/scr_player_fleet_functions.gml(1 hunks)scripts/scr_player_ship_functions/scr_player_ship_functions.gml(5 hunks)
🔇 Additional comments (6)
objects/obj_bomb_select/Alarm_1.gml (2)
2-6: Praising the initialization of dynamic arrays, Adeptus Astartes.
By clearing each array at the outset, thou ensure a pristine data vessel, well-suited for flexible expansions.
36-37: Sanctified approach to guard calculations.
Your invocation ofp_data.guard_score_calc()only whenguardsmen>0is neatly logical. Rejoice, for it is well-structured.scripts/scr_player_ship_functions/scr_player_ship_functions.gml (2)
289-289: Macro Bombardment Cannons upgraded with the Omnissiah’s favor.
Renaming to “Macro Bombardment Cannons” is consistent with the new bombardment logic. This pleases the Machine God.
426-450: Bombardment logic shows devotion to the Omnissiah.
Sub-scores are stored in a static structure—an efficient method. Continual updating should remain straightforward for adding future weapons.scripts/scr_player_fleet_functions/scr_player_fleet_functions.gml (2)
602-611: Validate ship IDs for negative values, revered Servitor.
While thou check that_ship_idis less thanarray_length(obj_ini.ship), failing to ensure_ship_id >= 0could lead to indexing misfortune.Wouldst thou like a script to confirm no negative IDs instigate warp anomalies?
614-623: Reiterating a venerable caution to ensure_ship_id >= 0.
This mirrors previous counsel from the archives. Negative IDs must be abhorred, lest they yield chaos in the warp.
Description of changes
Reasons for changes
Related links
How have you tested your changes?