feat(SemanticWorkflow): add loops (reconstructed)#114
Conversation
This commit adds loop functionality to the Semantic Workflow, allowing inputs to jump back to earlier inputs within the same section. Features: - Loop definition with count and jump target - Loop toggle button with icon in InputsTab - Loop count numberbox control - Loop target selection via special select handler - Loop counter reset on sheet run - Loop target highlighting in section drawing (orange border) - Loop source highlighting in section drawing (blue border) - Localization support (en_US, fr_FR) - Custom loop icon for light/dark themes Cleanup from original draft: - Removed debug print statements - Removed TODO comment about 'unclean' IndexOf usage - Added proper any_changes tracking for loop controls - Added run_to_preview() call after target selection - Extracted SectionInputs.new() into proper implementation file References: mupen64#109, mupen64#112
- Replace jump_target object reference with 1-based index to fix save/load serialization (circular reference in json.encode) - Add runtime guards in Sheet.evaluate_frame(): - validate target_index is within bounds and not a forward jump - handle nil runtime_counter safely - support count=0 as while-like infinite loop - Fix InputListGui highlighting to use composite keys (section_index:input_index) instead of object reference keys - Add section guard in InputsTab loop target selector - Initialize runtime_counter after load() if absent
FramePerfection
left a comment
FramePerfection
left a comment
There was a problem hiding this comment.
I'll take a deeper look into this later, but at a glance I think this may be fine.
There was a problem hiding this comment.
Same as in #112 (comment), this icon needs a license notice.
FramePerfection requested in review that the loop/loop_light icons (cycle from Google Material Symbols) carry a license notice. Refs: PR mupen64#114 review comment
FramePerfection requested in review that the loop/loop_light icons (cycle from Google Material Symbols) carry a license notice. Refs: PR mupen64#114 review comment
FramePerfection
left a comment
FramePerfection
left a comment
There was a problem hiding this comment.
A thing I hadn't properly considered that we should define good and ideally intuitive behavior is:
- nested loops (one loop entirely containing another)
- interlaced loops (a loop jumping back into the middle of a previous loop).
Perhaps the best course of action would be to reject those for now, as both cases aren't exactly trivial.
Code wise this looks decent, I'll take another look when the semantic issues are resolved, though.
| -- | ||
|
|
||
| ---@class Loop Runtime information for a section loop. | ||
| ---@field jump_target integer The 1-based index of the input to jump to within the same section. Can be the current input's index (self-loop), but must not be larger than the looping input's index. |
There was a problem hiding this comment.
I originally made this field a SectionInputs because that way the loop target correctly move along when inputs are added before it.
This functionality should be restored, by either:
- making this a
SectionInputsagain and only use indexing logic for the save/load routines, and handling deletion of the loop target explicitly - changing the target indices of looping inputs when inserting and deleting inputs before their respective targets
I'd personally probably actually go for the second option, now that I think about it.
| if loop_targets[section_index .. ":" .. input_index] then | ||
| BreitbandGraphics.draw_rectangle(section_rect, '#FF8000FF', 2) | ||
| end |
There was a problem hiding this comment.
I think it may be better to only draw the target rectangle for the currently selected looping input, if any, because drawing them all makes it impossible to distinguish which looping input is jumping where.
Point 2 (option B): When inserting or deleting inputs, adjust the jump_target indices of looping inputs in the same section. - Insert before => increment targets >= insert_index - Insert after => increment targets >= insert_index - Delete => decrement targets > delete_index, remove loop if target == delete_index Point 3: Only draw the orange target rectangle for the currently selected looping input, instead of all targets at once. This makes it possible to distinguish which loop jumps where. Refs: PR mupen64#114 review comments
FramePerfection requested that nested and interlaced loops be rejected as non-trivial for now. - InputsTab: add is_loop_target_valid() and gate special_select_handler so that selecting a loop target which would create a nested or interlaced loop is silently ignored. - Sheet: add is_loop_valid() and treat invalid loops as non-existent during playback, guarding against corrupted/legacy sheets. Refs: PR mupen64#114 review comments
There was a problem hiding this comment.
Everything seems to work correctly with my suggested overlap check change, so functionally I'd say this is ready for merge.
However, I do think it'd be nice if you wrote up a small tutorial page similar to the Atan one (which I admit I'll have to review for whether it's up to date with the actual implementation lol) on how to use the feature, specifically in regards to the color coding for now, because I feel like that can be confusing without explanation.
On a related note, I've noticed that the text lines in the help page don't wrap anymore like they used to, which is another issue (potentially related to #86 (EDIT: yeah, 5b99324 broke it, refer to #120)), but that's a different issue not related to writing the text itself.
| ---@param section Section | ||
| ---@param own_index integer | ||
| ---@param new_target integer | ||
| ---@return boolean | ||
| local function is_loop_valid(section, own_index, new_target) | ||
| for other_index, other_input in ipairs(section.inputs) do | ||
| if other_index ~= own_index and other_input.loop then | ||
| local other_target = other_input.loop.jump_target | ||
| if other_target then | ||
| local a, b = new_target, own_index | ||
| local c, d = other_target, other_index | ||
| local nested = (a < c and d < b) or (c < a and b < d) | ||
| local interlaced = (a < c and c < b and b < d) or (c < a and a < d and d < b) | ||
| if nested or interlaced then | ||
| return false | ||
| end | ||
| end | ||
| end | ||
| end | ||
| return true | ||
| end | ||
|
|
There was a problem hiding this comment.
This is a near exact duplicate of the (faulty) logic in is_loop_target_valid. Either extract the core logic, or remove the validation at this stage entirely, as sheet validation is a separate concern to be addressed at a later time. (See "Proposed Enhancement" in #26)
| if target_index == nil or target_index < 1 or target_index > #section.inputs | ||
| or target_index > self._input_index | ||
| or (loop.count ~= 0 and runtime_counter >= loop.count) | ||
| or not is_loop_valid(section, self._input_index, target_index) |
There was a problem hiding this comment.
See my other comment regarding this validation.
More apparently here, though: I don't see validation having a place in evaluation logic whatsoever, since that's a hot path and much more performance critical than "loading" logic (i.e. opening a project from the drive), especially with how validation logic tends to be expensive itself (this one contains a loop over the entire section's input range).
If you wish to address file validation to catch corrupted or "legacy" files, do so in a separate PR.
Do note, however, that the current model of the file format is intentionally left loose for now to allow quick evolution, only insisting on the semantic versioning model that's described in the help page (which explicitly allows "catastrophically" (i.e. script crashing) breaking changes between major versions).
Co-authored-by: Frame <Nafoizret@web.de>
…hugou74130/SM64LuaRedux into feat/semantic-workflow-loops-v2
2 participants
Summary
This PR reconstructs the loop functionality on top of the current main branch, which already includes the semantic input changes from #100.
Background
The original loop draft in #109 was based on an older branch that has since been largely merged via #100. This PR extracts only the loop-specific changes and cleans them up for production.
Changes from #112 incorporated
Cleanup from original draft
print()statementsany_changestracking for loop controlsrun_to_preview()call after loop target selectionSectionInputs.new()into proper implementation fileTechnical Details
Loopclass definition inSectionInputs.luawithjump_target,count, andruntime_counterfieldsSheet.luaevaluate_frame()— when an input with a loop reaches its end condition, it jumps back to the target input instead of advancing, until the loop count is exhaustedrun_to_preview()is calledCloses #112