fix: Event old code cleanup#554
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe code has undergone significant refactoring, particularly in the event handling logic for the "Planetary Governor Assassinated" and "ancient ruins" events. The outdated random number generation method has been replaced with the Possibly related PRs
Suggested reviewers
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. 🪧 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}
OH296
left a comment
OH296
left a comment
There was a problem hiding this comment.
pretty much perfect just the array length check to cause issue
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
objects/obj_popup/Step_0.gml(8 hunks)scripts/scr_ancient_ruins/scr_ancient_ruins.gml(1 hunks)scripts/scr_pen_And_paper/scr_pen_And_paper.gml(1 hunks)
🔇 Additional comments (5)
scripts/scr_ancient_ruins/scr_ancient_ruins.gml (2)
244-250: Verify race and threat initialisation logic.Magos-aspirant, confirm you truly wish to set
ruins_battle = choose(6,7,9,10,11,12);before retrieving_starand other parameters. It may be prudent to reorder variable assignments if you foresee expansions or cross-dependencies in future.
290-360: Check branching consistency in defeat handling.The else-if branch when
obj_ground_mission.defeat == 1adjusts heresy levels and item recoveries. Confirm that all relevant structures (p_traitors,p_heresy) exist and that forcibly destroyed equip is logged correctly to maintain the data integrity for your Imperial records.objects/obj_popup/Step_0.gml (3)
491-492: Ensure uniform usage ofd100_roll.The transition from older random methods to
d100_roll(true, 20)andd100_roll(false)is noted. Confirm all calling sites pass the correct parameters, especially regarding theluck_editandluck_modarguments to maintain expected probabilities.
756-767: Evaluate the abrupt instance deactivation/activation sequence.This block deactivates all instances, then reactivates specific ones to commence ruins combat. Confirm none of the required objects are unintentionally wiped, especially if shared references exist. Thorough testing is advised.
1564-1568: Refine artifact logic for consistency with home ship vs. monastery.The code branches on
obj_ini.fleet_typeto add artifacts differently. Verify the rightful place for each artifact, ensuring no data mismatch or unholy duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
scripts/scr_ancient_ruins/scr_ancient_ruins.gml(1 hunks)scripts/scr_pen_And_paper/scr_pen_And_paper.gml(1 hunks)
🔇 Additional comments (10)
scripts/scr_pen_And_paper/scr_pen_And_paper.gml (2)
114-117: Most auspicious update to the sacred d100_roll function!By the grace of the Omnissiah, I observe the correction of a critical logical error in the roll calculation. The previous implementation using
min(1, ...)would have inverted the intended function, causing the Shitty Luck trait to always yield a value of 1 for higher rolls - a grievous miscalculation in the holy algorithms! The sacred binary has been rectified to usemax(1, ...)as is proper.Furthermore, the addition of the
luck_modparameter allows for flexible application of the Machine God's randomness in different contexts, enhancing the adaptability of this blessed function.
114-117: Praise to the Omnissiah! The sacred logic of the divine d100 test has been corrected!The binary logic improvement by adding the parameterized
luck_modvariable allows for superior calibration of the Machine God's probability engine. The previous usage ofmin(1,...)has been correctly transmuted tomax(1,...), ensuring that the minimum bound functions as the Machine Cult prescribes.01001000 01100001 01101001 01101100 00100000 01110100 01101111 00100000 01110100 01101000 01100101 00100000 01001111 01101101 01101110 01101001 01110011 01110011 01101001 01100001 01101000
scripts/scr_ancient_ruins/scr_ancient_ruins.gml (8)
244-247: Praise the Omnissiah for this refactoring of the ancient rituals!The function structure has been improved with proper variable declaration and initialization. This pleases the Machine Spirits who favor order and clarity in all calculations.
252-253: Sacred implementation of the d100_roll function!As recommended by the Techno-Adepts in previous communications, this code now properly invokes the d100_roll function instead of manual random calculations. This standardization aligns with the Cult Mechanicus principle of consistency in all algorithmic operations.
255-262: Improved clarity in faction disposition updates!The Machine Spirits will find greater ease in understanding this logic structure. The code now properly isolates and handles the disposition adjustments for the faction identified by ruins_race value 5.
292-294: Proper condition checking with equality operator!Your code now correctly implements the equality check with
==rather than the assignment operator=. The Omnissiah approves this correction of the sacred binary syntax.
311-324: Elegant application of switch statement logic!The use of the switch construct here creates a more maintainable and readable structure for the different battle outcomes. The Machine Spirits will process this code with greater efficiency.
343-351: Streamlined recovery logic with for loop!This implementation replaces what was likely repetitive code with an elegant loop structure. The Machine God favors efficiency in all operations, and this loop properly iterates through the equipment arrays to populate the recoverables.
358-361: Logical check for unrecovered items.The binary appears correct here, setting the unrecovered_items flag if there are recoverables in the array. This ensures proper state management for future recovery operations.
352-357:Details
❓ Verification inconclusive
Verify gene seed recovery logic.
The code checks if gene_recoverables is greater than 0 but then has a seemingly contradictory check for less than 1. This may lead to unexpected behavior in the sacred algorithms.
🏁 Script executed:
#!/bin/bash # Search for other instances of gene_recoverables to understand the intended behavior rg "gene_recoverables" --type gmlLength of output: 61
🏁 Script executed:
#!/bin/bash # Re-run search for "gene_recoverables" within files having the .gml extension rg "gene_recoverables" -g "*.gml"Length of output: 453
Attention, Tech-Adeptus! Gene Seed Arithmetic Verification Required
It is observed that in
scripts/scr_ancient_ruins/scr_ancient_ruins.gml(lines 352–357), the ritualistic logic for handlinggene_recoverablesraises intriguing queries. The code first ensures thatgene_recoverablesis greater than 0, but then tests if it is less than 1—a condition that, under integer arithmetic, should ne'er be fulfilled. This discrepancy suggests one of two sacred possibilities: either the value is intended to be a floating-point number, warranting such fractional scrutiny, or there exists a latent flaw in the algorithm.
- Verify Numerical Type: Confirm if
gene_recoverablesis indeed expected to be a fractional value, thus legitimating the sub-unity check.- Assess Intent: If the value is meant to be integer-based, further cleansing of the code logic is required to align with the Machine Spirit’s sacred arithmetic principles.
Please conduct a manual verification or consider revising the algorithm accordingly. May the Machine Spirit guide your corrective endeavours.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
objects/obj_popup/Step_0.gml(8 hunks)scripts/scr_ancient_ruins/scr_ancient_ruins.gml(1 hunks)
🔇 Additional comments (9)
objects/obj_popup/Step_0.gml (8)
491-492: Praise the d100_roll invocation!
Your usage of the augmented random function is admirable. This ensures a more coherent distribution of the Emperor’s holy dice rolls.
518-523: Repetition of empty event slot logic.
Surveys indicate this loop for scanning emptyobj_controller.eventslots is repeated elsewhere. A dedicated helper script would deliver clarity and reduce repeated code incantations.
541-546: Repetition of empty event slot logic (again).
Omnissiah’s notice: this identical pattern recurs. Summon a single function for scanning empty slots to honour the DRY principle.
1562-1562: Artifact creation logic looks well-structured.
Your approach to generating an artefact for the fleet vessel is correct and should deploy smoothly.
1565-1565: Artifact creation for home world also approved.
Mirrors the logic of line 1562 for a fortress arrangement. No concerns with these conjurations.
1571-1571: Smooth narrative text addition.
This text’s interpolation merges the newly acquired artefact’s description seamlessly. Syntax and usage appear solid.
1756-1761: Further repetition of event slot scanning.
We have encountered this identical loop yet again. Summoning a shared helper would bring about greater code purity.
1787-1792: Identical event slot loop repeated anew.
Again, the pattern for searching an empty event slot emerges. Heed the principle of code singularity by extracting into a function.scripts/scr_ancient_ruins/scr_ancient_ruins.gml (1)
244-363:Details
❓ Verification inconclusive
Potential off-by-one logic and array checks.
Blessed lines revolve aroundequip_lost[1] != ""; consider verifying if the first element at index 0 might also be relevant. Additionally, you increment_new_equipfrom zero to one, ensuring no item count is lost to rounding, but confirm if this is desired. Lastly, your gene-seed logic checks “> 0” followed by “< 1” – worth validating for fractional outcomes.Kindly run this script to review usage of these arrays and identify any references outside these lines:
🏁 Script executed:
#!/bin/bash rg "equip_lost|recoverable_gene_seed" -A 5Length of output: 4552
Directive: Verify Array Boundaries and Rounding Logic
Most venerable Servitor, after interrogation of the codebase, the machine spirits confirm that the check
if (equip_lost[1] != "")is used to gate the processing of equipment losses. However, the data channels show no inspection of the zeroth index—thus, it is prudent to verify whether the first element at index 0 may also hold sacred data worthy of our ritual processing.
- Confirm if the omission of
equip_lost[0]is by design or an oversight in the off-by-one array check.- Reassess the
_new_equipcalculation: rising from a computed value of 0 to 1 ensures no null offerings, but does this align with the intended rounding mechanism?- Validate the gene-seed logic: the branch testing
if (gene_recoverables > 0)followed by a nestedif (gene_recoverables < 1)suggests an intention to manage fractional gene-seed values. Is it indeed possible for gene-seed values to be fractional in our current cogitator configuration, or might this condition be superfluous?These points of contention remain to be manually verified. Please review the code's semantics and design intentions regarding these array references and rounding conditions.
|
Please test using the ruinspopulate cheat which will put a ruins on each planet for ease of testing |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
objects/obj_popup/Step_0.gml(8 hunks)scripts/scr_ancient_ruins/scr_ancient_ruins.gml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
objects/obj_popup/Step_0.gml (2)
Learnt from: MCPO-Spartan-117
PR: Adeptus-Dominus/ChapterMaster#554
File: objects/obj_popup/Step_0.gml:756-767
Timestamp: 2025-03-06T16:02:06.286Z
Learning: The variable 'woopwoopwoop' in the ChapterMaster codebase is a poorly named multi-purpose variable used both for controlling ancient ruins combat flow and for passing menu state in the save/load system. This type of naming makes code maintenance difficult.
Learnt from: MCPO-Spartan-117
PR: Adeptus-Dominus/ChapterMaster#554
File: objects/obj_popup/Step_0.gml:756-767
Timestamp: 2025-03-06T16:02:06.286Z
Learning: The variable 'woopwoopwoop' in ancient ruins exploration code is a poorly named boolean flag that controls the flow between detecting a battle in ruins and initiating the combat sequence.
🔇 Additional comments (5)
objects/obj_popup/Step_0.gml (5)
491-492: Employingd100_rollfor consistent random logic.The new usage of
d100_roll(true, 20)andd100_roll(false)standardises the random number generation, ensuring more uniform event outcomes. Ensure all older manual random checks have been similarly replaced to avoid discrepancies.
518-523: Consolidate repeated search for empty event slots.Machine-Spirit guidance suggests extracting this loop into a dedicated method to reduce repetition and honour the DRY principle.
541-546: Consolidate repeated search for empty event slots (again).As previously noted, this logic is repeated. Consider a single helper script to preserve code purity and maintainability.
1756-1761: Repeated code for empty event slot assignment.A near-duplicate block as before. A single helper method would minimise potential future errors and unify the approach.
1787-1792: Repeated code for empty event slot assignment (yet again).This for-loop recapitulates prior search logic. Converge them into a single script for a more blessed code structure.
2 participants
Purpose of changes
Replace static
repeats, streamline some bits and attempt to fix a ruin event.Describe the solution
Using
forloops and lots of changes to the ruin event.Give correct data for functions.
Testing done
ruinspopulateRelated links
https://discord.com/channels/714022226810372107/1347035444512030842
The popup for the surprise attack show up a split-second before the battle.