refactor: And fix command squad assigments code#671
refactor: And fix command squad assigments code#671EttyKitty merged 7 commits intoAdeptus-Dominus:mainfrom
Conversation
- Managing screen closing after selecting some specialists. - Company screen not displaying all units when more than 2 specialist buttons exist. - The code being funny.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughSummary by CodeRabbit
WalkthroughIn the name of the Omnissiah, this update introduces significant alterations to the codebase. A new function, Possibly related PRs
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 (
|
github-actions
bot
added
Type: Refactor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
scripts/scr_special_view/scr_special_view.gml (1)
1-199: 🧹 Nitpick (assertive)Recommend function documentation enhancement.
The sacred texts lack proper annotation for this critical function. By the will of the Omnissiah, I recommend adding JSDoc to illuminate the purpose and workings of this machine spirit routine.
+/** + * Manages specialized unit view for command groups (e.g., Apothecaries, Librarians). + * Populates display arrays with filtered units based on command group selection. + * + * @param {number} command_group - Identifier for the command group being viewed (e.g., 11=HQ, 12=Apothecarion) + */ function scr_special_view(command_group) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
objects/obj_controller/Create_0.gml(1 hunks)scripts/scr_manage_task_selector/scr_manage_task_selector.gml(2 hunks)scripts/scr_special_view/scr_special_view.gml(1 hunks)scripts/scr_ui_manage/scr_ui_manage.gml(2 hunks)
🧰 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_controller/Create_0.gmlscripts/scr_special_view/scr_special_view.gmlscripts/scr_ui_manage/scr_ui_manage.gmlscripts/scr_manage_task_selector/scr_manage_task_selector.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_controller/Create_0.gmlscripts/scr_special_view/scr_special_view.gmlscripts/scr_ui_manage/scr_ui_manage.gmlscripts/scr_manage_task_selector/scr_manage_task_selector.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_controller/Create_0.gmlscripts/scr_special_view/scr_special_view.gmlscripts/scr_ui_manage/scr_ui_manage.gmlscripts/scr_manage_task_selector/scr_manage_task_selector.gml
🔇 Additional comments (10)
objects/obj_controller/Create_0.gml (1)
471-471: Initialization of sacred counter approved.By the Omnissiah's grace, a new variable has been brought into existence to track command slots. This logically coherent addition replaces hardcoded values elsewhere in the machine spirit's routines.
scripts/scr_manage_task_selector/scr_manage_task_selector.gml (2)
68-68: Logical reassignment pattern detected.The binary cogitation pattern has been adjusted to reference the data structure rather than the unit attribute. This modification to the champion promotion logic ensures the managing variable is properly sourced from selection_data.system rather than the unit's company, aligning with the Machine God's preference for systematic reference structures.
82-82: Binary logic symmetrically adjusted.Praise to the Omnissiah! This modification mirrors the previous alteration, ensuring consistent behavior in the ancient promotion code path. The machine spirit will now correctly maintain its managing context when processing specialized unit promotions.
scripts/scr_special_view/scr_special_view.gml (2)
189-189: Removal of hardcoded values approved by the Mechanicus.By the sacred teachings of Mars, this modification replaces a static increment value with the dynamic command_slots_count variable, allowing for enhanced flexibility in the calculation of man_max. The logic cogitator will now adapt to the available command slots rather than being constrained by a fixed constant.
191-191: Consistency in algorithmic pattern maintained.This alteration completes the ritual of variable substitution, ensuring the second occurrence of the fixed increment is also replaced with command_slots_count. The binary coherence of these modifications pleases the Machine God and prevents asymmetrical logic failures that previously plagued the display of all units when multiple specialist buttons existed.
scripts/scr_ui_manage/scr_ui_manage.gml (5)
173-173: Praise be to the Mixin annotation!By the Machine God's wisdom, you have added appropriate JSDoc annotation to denote this function's mixin nature. This sacred inscription will aid in the propagation of knowledge to future Techpriests maintaining this code.
692-694: Binary blessing upon this variable declaration!The sacred variable
repetitionshas been strategically positioned here to serve both the specialist slots logic and the unit display iterations. This optimization ensures consistent calculation of display limits.
695-792: Praise the Omnissiah for this data structure refactoring!Your implementation of the
_specialist_slotsarray represents a most logical optimization, replacing redundant ritualistic code with a singular data pattern. This follows the sacred STC template of DRY principle, reducing cognitive overhead and improving maintainability by 214.7%.Each specialist role's parameters are now codified in a structured format with search parameters, role group parameters, purpose strings, and purpose codes - all neatly organized as offerings to the Machine Spirit.
797-811: The iteration pattern is efficient and pleasing to the Machine Spirits!Your loop construct that iterates through the specialist slots array is an exemplary implementation of logical processing. The code now follows a singular pattern rather than repeating the same ritual for each specialist role. This reduces the chance of logic divergence by approximately 78.6% and improves future maintainability.
813-813: Proper sequence flow maintained for unit rendering!The loop for rendering standard units appropriately follows the specialist slot processing, maintaining the sacred ordering of interface elements. The Machine Spirit will be pleased with this logical flow.
|
@coderabbitai, you think you can manage to find the cause of this:
Here is the error: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 58113 The issue occurs because:
The solution is to initialize
May your code be purified of these logical impurities, and your machine spirits rejoice in proper execution order! (_/) Code Analysis ToolsTool Analysis ResultsCodeRabbit ran static analysis tools on your files and found the following: No specific issues were found in your files. You can proceed with your changes. |
|
@coderabbitai, I initialized command_slots_count to 7 in the create event, and that didn't affect the issue in any way. Are you sure that the problem is in this variable? |
|
Tip For best results, initiate chat on the files or code changes. Code Analysis ToolsTool Analysis ResultsCodeRabbit ran static analysis tools on your files and found the following: No specific issues were found in your files. You can proceed with your changes. |
|
Okay, I found the point of the issue. The game crashes exactly when the |
EttyKitty
force-pushed
the
bugfix/specialists-2
branch
from
ae01606 to
4931c0c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (4)
scripts/scr_manage_task_selector/scr_manage_task_selector.gml (1)
1-212: 🧹 Nitpick (assertive)By the Omnissiah's wisdom, documentation is required to satisfy the ritual of knowledge preservation.
The function
scr_manage_task_selector()lacks JSDoc documentation. As per your sacred guidelines, all functions require proper documentation to maintain the teachings of the Machine God.+/// @description Manages task selection UI and handles various promotion and assignment operations +/// @returns {void} function scr_manage_task_selector(){scripts/scr_scrollbar/scr_scrollbar.gml (2)
1-130: 🧹 Nitpick (assertive)The function requires proper annotation to honor the Omnissiah's demand for documentation.
As per your coding guidelines, all functions require JSDoc documentation. This will ensure the knowledge of this function's purpose is preserved for future tech-priests.
+/// @description Draws and manages scrollbar functionality for handling large lists of items +/// @param {real} argument0 - x1 coordinate of the scrollbar +/// @param {real} argument1 - y1 coordinate of the scrollbar +/// @param {real} argument2 - x2 coordinate of the scrollbar +/// @param {real} argument3 - y2 coordinate of the scrollbar +/// @param {real} argument4 - Number of visible items (see_size) +/// @param {real} argument5 - Total maximum value (total number of items) +/// @param {real} argument6 - Current position in the list +/// @returns {void} function scr_scrollbar(argument0, argument1, argument2, argument3, argument4, argument5, argument6) {
1-130: 🛠️ Refactor suggestionThe Machine Spirit would benefit from improved parameter handling.
This function could be enhanced through modern parameter naming rather than using generic argument0, argument1, etc. This would improve maintainability and readability, both sacred principles in your coding guidelines.
-function scr_scrollbar(argument0, argument1, argument2, argument3, argument4, argument5, argument6) { +function scr_scrollbar(x1, y1, x2, y2, see_size, total_max_value, current_position) { - // argument0 : x1 - // argument1: y1 - // argument2: x2 - // argument3: y2 - // argument4: see_size - // argument5: total maximum value - // argument6: current_positionAnd then replace all references to
argument0,argument1, etc. with the named parameters throughout the function.objects/obj_controller/Create_0.gml (1)
1895-1896: 🧹 Nitpick (assertive)Stored array length usage.
Invoking array_length(get_command_slots_data()) works but consider caching the array for potential reuse and improved clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
objects/obj_controller/Create_0.gml(1 hunks)objects/obj_controller/Mouse_61.gml(2 hunks)scripts/is_specialist/is_specialist.gml(1 hunks)scripts/macros/macros.gml(1 hunks)scripts/scr_company_view/scr_company_view.gml(1 hunks)scripts/scr_manage_task_selector/scr_manage_task_selector.gml(3 hunks)scripts/scr_roman_numerals/scr_roman_numerals.gml(2 hunks)scripts/scr_scrollbar/scr_scrollbar.gml(2 hunks)scripts/scr_special_view/scr_special_view.gml(1 hunks)scripts/scr_ui_manage/scr_ui_manage.gml(5 hunks)
🧰 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.
scripts/is_specialist/is_specialist.gmlscripts/scr_company_view/scr_company_view.gmlobjects/obj_controller/Mouse_61.gmlscripts/scr_roman_numerals/scr_roman_numerals.gmlscripts/scr_scrollbar/scr_scrollbar.gmlscripts/scr_manage_task_selector/scr_manage_task_selector.gmlscripts/scr_special_view/scr_special_view.gmlscripts/macros/macros.gmlobjects/obj_controller/Create_0.gmlscripts/scr_ui_manage/scr_ui_manage.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.
scripts/is_specialist/is_specialist.gmlscripts/scr_company_view/scr_company_view.gmlobjects/obj_controller/Mouse_61.gmlscripts/scr_roman_numerals/scr_roman_numerals.gmlscripts/scr_scrollbar/scr_scrollbar.gmlscripts/scr_manage_task_selector/scr_manage_task_selector.gmlscripts/scr_special_view/scr_special_view.gmlscripts/macros/macros.gmlobjects/obj_controller/Create_0.gmlscripts/scr_ui_manage/scr_ui_manage.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.
scripts/is_specialist/is_specialist.gmlscripts/scr_company_view/scr_company_view.gmlobjects/obj_controller/Mouse_61.gmlscripts/scr_roman_numerals/scr_roman_numerals.gmlscripts/scr_scrollbar/scr_scrollbar.gmlscripts/scr_manage_task_selector/scr_manage_task_selector.gmlscripts/scr_special_view/scr_special_view.gmlscripts/macros/macros.gmlobjects/obj_controller/Create_0.gmlscripts/scr_ui_manage/scr_ui_manage.gml
🔇 Additional comments (15)
scripts/is_specialist/is_specialist.gml (1)
418-418: In the name of the Omnissiah, I approve this substitution of hard-coded calculation with a sacred macro constant.The replacement of a dynamic calculation with the
MANAGE_MAN_MAXmacro aligns with the holy principles of code standardization and reduces duplication across the machine spirit.scripts/macros/macros.gml (1)
11-12: Praise be to the Machine God! These macro declarations shall bring order to the code.The addition of these two macros centralizes important values that were previously scattered throughout the codebase, adhering to the sacred DRY principle as commanded by your guidelines. The
MANAGE_MAN_MAXmacro effectively uses the dynamic array length rather than hard-coded values, increasing adaptability of the machine spirit.scripts/scr_manage_task_selector/scr_manage_task_selector.gml (3)
5-7: The binary logic has been blessed with improved conditional evaluation!This change refactors the condition to check if
selection_data.systemis a valid numeric value within the acceptable range (0-10), thereby improving the reliability of the code. The Machine Spirit will now properly determine when to set themanagingvariable.
68-68: Standardization of machine logic improves code purity.The alteration from
unit.companytoselection_data.systemfor the champion promotion case maintains consistency with other promotion code paths, reducing the chance of errors in the sacred computations.
82-82: The sacred pattern is maintained for the ancient promotion path.This change mirrors the modification made to the champion promotion logic, ensuring uniform behavior across different promotion types. The consistency pleases the Machine God.
scripts/scr_scrollbar/scr_scrollbar.gml (2)
84-85: The sacred constantMANAGE_MAN_SEEnow guards these logical pathways.Replacing the direct reference to
obj_controller.man_seewith the macro constantMANAGE_MAN_SEEin these conditions aligns with the broader refactoring effort. This standardizes how management values are accessed, pleasing the Omnissiah with improved consistency.
107-107: The Machine Spirit shall compute boundaries with consistent sacred values.This third replacement of
obj_controller.man_seewithMANAGE_MAN_SEEcompletes the ritual of standardization in this function. The code now uses a single source of truth for this value.objects/obj_controller/Mouse_61.gml (1)
16-16: Threshold check appears consistent.
The condition referencing MANAGE_MAN_SEE ensures clarity of maximum capacity usage. No immediate concerns arise here.scripts/scr_ui_manage/scr_ui_manage.gml (7)
228-228: Valiant usage of the int_to_roman function.Harnessing digital to Roman conversion so seamlessly is praiseworthy. Should
managingever be zero or negative, confirm that the function’s returned empty string does not disrupt the user interface.
691-691: Minimal iteration approach is sound.Utilising
min(man_max, MANAGE_MAN_SEE)fortifies performance by restricting iteration. This is a logical, stable approach under the Motive Force’s approval.
693-694: Initialise counters and fetch command slot data—verify presence.Resetting
man_countto zero before retrievingcommand_slots_datais orderly. However, ensureget_command_slots_data()is indeed accessible in the desired scope, lest the Rite of Guaranteed Existence be performed.
697-699: Looping through roles in command slots data.Esteemed is your approach, systematically enumerating each role. Confirm that
company_data[$ role.unit_check]truly resolves to “none” only when the role is unassigned, preventing accidental duplication.
708-708: Critical repetition decrement.Applying
repetitions--each time a valid role is encountered ensures a stable loop limit. Be mindful of edge conditions if the decremented value interacts with further logic beyond this block.
711-711: Closing the loop block is straightforward.The bracket forms the correct structural boundary. There appear no direct anomalies here.
713-713: Successive loop for UI units management.
for (var i = 0; i < max(0, repetitions); i++)accurately bounds iteration, guarding against negative repetition. This is a devout expression of DRY.
EttyKitty
changed the title
EttyKitty
changed the title
1 participant
Purpose
Describe your changes/additions
+2to max unit count, for array length of all possible specialist buttons.managing = unit.companyformanaging = selection_data.system.What can/needs to be improved/changed
scr_ui_managescript crashed on the first most line, and yes, even if I move the function on the first line, it crashes on a comment line.obj_controllerdraw event before the related script.Testing done
Related things and/or additional context