Fix: quickfort UI freeze on large blueprints and strip undocumented ghost-features#1597
Fix: quickfort UI freeze on large blueprints and strip undocumented ghost-features#1597sizzlins wants to merge 1 commit into
Conversation
|
This PR seems to combine two unrelated changes, one related to quickfort and one implementing some sort of manual-save feature, possibly the same as the one in #1583. Please don't bundle unrelated changes into a single PR. |
sizzlins
force-pushed
the
fix/quickfort-lag-and-ghost-code
branch
5 times, most recently
from
292b40a to
e6ab51d
Compare
sizzlins
force-pushed
the
fix/quickfort-lag-and-ghost-code
branch
from
e6ab51d to
ad9eac2
Compare
SilasD
left a comment
SilasD
left a comment
There was a problem hiding this comment.
the code for for the main feature of the PR, the delayed-refresh looks kind of reasonable, I guess? some odd idioms.
thing is, I don't see much difference between refreshing the preview every time the mouse cursor enters a different tile or waiting 50ms and then refreshing. yes, the mouse cursor can move multiple tiles in 50ms, but only if the blueprint is dragged around very quickly.
my gut feeling is that it doesn't really matter whether the CSV is fully reparsed now or fully reparsed in 50 ms, it's going to be fully reparsed anyway. and probably this happens on every tile the mouse cursor moves onto.
if you want to speed up dragging around a blueprint, targetting that parsing seems like the path to take. something as simple as saving an intermediate representation could do a lot of good.
but it feels like a tempest in a teapot.
for the other changes, they seem to be a TODO implemented without understanding the consequences, deletion of beneficial code, and a "fix" of "undocumented ghost-features" which actually breaks a documented and working feature.
recommend reject, of course.
and I gave this well over an hour of my time. now I see why professionals are auto-rejecting anything that smacks of "AI slop".
| widgets.HotkeyLabel{ | ||
| frame={b=0, l=15}, | ||
| key='SELECT_ALL', -- TODO: change to SEC_SELECT once 51.01 is stable | ||
| key='SEC_SELECT', |
There was a problem hiding this comment.
has this been tested? can you explain what key sequence or mouse button presses this would change?
it's not enough to blithely say, oh yeah, we're way past 51.01 so let's just perform the TODO while we're in here.
SEC_SELECT showed up as a keybinding/mousebinding between 50.15 and 51.02. what is it? is SELECT_ALL deprecated now? what is being changed here?
should this be merged? I'm honestly not sure. I don't think it would hurt, but I don't know that there's a reason to do it.
| args.commands = {action} | ||
|
|
||
| local action_fn = action_switch[args.commands[1]] | ||
| local action_fn = action_switch[action] |
There was a problem hiding this comment.
the issue this purports to fix is real, although I don't know if it's severe enough to warrant a fix.
this proposed fix is wrong; it removes a documented capability for no good reason.
"quickfort <command>[,<command>...] <list_id>[,<list_id>...] [<options>]"
Applies the blueprint(s) with the id number(s) reported from the "list" command.
"quickfort <command>[,<command>...] <filename> [-n|--name <name>[,<name>...]] [<options>]"
Applies a blueprint in the specified file. {{ details removed, not relevant }}
quickfort is designed to accept multiple comma-separated commands. this proposed fix removes that capability.
I recommend against merging this particular change.
Edit: the fact that this capability is intentional and documented speaks against the title of this PR. "strip undocumented ghost-features". no.
|
|
||
| local origin, test_point = {x=0, y=0}, {x=1, y=-2} | ||
| local minimal_sequence = { | ||
| ['x=1, y=-2'] = {}, | ||
| ['x=2, y=-1'] = {'cw', 'flipv'}, | ||
| ['x=2, y=1'] = {'cw'}, | ||
| ['x=1, y=2'] = {'flipv'}, | ||
| ['x=-1, y=2'] = {'cw', 'cw'}, | ||
| ['x=-2, y=1'] = {'ccw', 'flipv'}, | ||
| ['x=-2, y=-1'] = {'ccw'}, | ||
| ['x=-1, y=-2'] = {'fliph'} | ||
| } | ||
|
|
||
| -- reduces the list of transformations to a minimal sequence | ||
| local function reduce_transform(elements) | ||
| local pos = test_point | ||
| for _,elem in ipairs(elements) do | ||
| pos = quickfort_transform.make_transform_fn_from_name(elem)(pos, origin) | ||
| end | ||
| local ret = quickfort_transform.resolve_vector(pos, minimal_sequence) | ||
| if #ret == #elements then | ||
| -- if we're not making the sequence any shorter, prefer the existing set | ||
| return elements | ||
| end | ||
| return copyall(ret) | ||
| end | ||
|
|
||
| function Quickfort:on_transform(val) | ||
| table.insert(transformations, val) | ||
| transformations = reduce_transform(transformations) | ||
| self:updateLayout() |
There was a problem hiding this comment.
I don't see any reason to remove this code. without it, the UI Label would grow without bound as the user applies multiple transformations.
this code is beneficial, and it only runs when the user applies a transformation, so it doesn't impact the performance per the stated intent of the PR.
| local now = os.clock() | ||
| -- wait at least 0.05s (50ms) between heavy recalculations | ||
| if not self.last_refresh_time or (now - self.last_refresh_time > 0.05) then |
There was a problem hiding this comment.
I think I'd prefer this be based on frames instead of wall clock. ticks are of course not relevant, because ZScreen.
3 participants
Title:
Fix: quickfort UI freeze on massive blueprints and strip undocumented ghost-featuresThis PR addresses severe UI lag when selecting massive blueprints (e.g.
industry2) in thegui/quickfortoverlay, and removes broken, undocumented ghost-code from the corequickfort.luaparser.Fixed massive UI render lag
onRenderFramewas synchronously re-parsing the blueprint CSV and reevaluating thousands of matrix tiles 60 times a second whenever the mouse moved. This completely froze the game engine.dx, dy, dz) to the cached preview tiles. The blueprint now perfectly tracks the mouse at a smooth 60 FPS while the heavy collision math runs asynchronously in the background.Removed
argparse.stringListThe
actionargument was previously being run throughargparse.stringList(action). This undocumented feature bypassed the safety mechanism (not dfhack.isMapLoaded()) because the literalactionstring check (action == 'run') fails if the string is"run,orders".Removed
reduce_transform()The UI had a 25-line mathematical
reduce_transform()block dedicated purely to minimizing the length of a UI display text label when a user hit the rotate hotkey multiple times.Deleted the function to simplify the UI loop.
transformationsnow behaves as a standard array.Updated
SELECT_ALLtoSEC_SELECTas the 51.01 transition has been stable for multiple major releases.