Fix some papercuts, 2024 edition#1729
Merged
Merged
Conversation
I frequently change something, tab back to the map editor, get this prompt, accept, and then have the focus moved to the main window. Feels very silly.
…on#1722) Apparently the GTK devs have decided that synchronously ticking the event loop whenever you want isn't allowed, so the GTK function that wx calls from Update() no longer actually forces a repaint. Luckily, wx's family of Yield functions addresses this, by... er... synchronously ticking the event loop. Well, okay. One problem here: during app init (specifically wxApp::OnInit), the event loop doesn't exist yet, so this still doesn't work. I think the correct approach might be to move most of the setup to wxApp::OnEventLoopEnter (checking for loop->IsMain()) so we know it exists, but that's a more significant change and I'm not sure of the ramifications.
At least on GTK, this avoids giving the splash window its own (untitled, icon-less) taskbar entry, and also prevents clicking on the main window and accidentally raising it above the splash window. It does mean the splash window needs to be recreated after app init, but it's extremely cheap so that should be fine.
Apparently ZDoom ignores the sign when combining these. Handily, that means left/right will visually move the texture left/right, even with negative scale.
I had some architecture with vertices that were slightly off integer coordinates -- I think perhaps some float cruft crept in after using Edit objects? -- and simply moving it caused the "split lines that moved onto vertices" loop to run forever. You can replicate this by drawing two boxes (in a UDMF map), manually editing one corner of one box (call it A) to have a coordinate 0.05 off an integer, and then dragging a corner of the second box (call it B) onto it. You'll get stuck as follows: 1. One of the affected edges of the second box (BC) will be detected as being within 0.1 units of A, and so that line will be split. 2. The split will create two lines: AB and AC. AB is only 0.05 units long, and AC is nearly identical to the original line. 3. These new lines will be added to `connected_lines` so the loop can examine them for any further vertex collisions. 4. On a later iteration, AC will be detected as being within 0.1 units of B, and so it will be split -- into a duplicate AB and BC. These will also be added to `connected_lines`, and so on. The simplest fix seems to be to make `mergeVerticesPoint` use the same fuzz radius as the line-splitting, so the two vertices are collapsed before any of this can happen. That said, float precision makes it possible that this could still happen under extremely specific circumstances. A more robust fix might be to identify duplicate lines upfront and also as they're created, but that's a bit more complicated.
At first glance this looked like it would re-run for every modified sector, but on second thought, all of them would have vbo_update_ = 0 afterwards. Still, I already wrote it and this is easier to follow.
The last sector would get swapped into its slot, which would screw up the VBO ordering -- at best the last sector's flat would render where the deleted sector had been, at worst the number of vertices would be different and all kinds of broken polygons would result. Simple fix is to manually invalidate.
Previously, the prop grid was populated in std::map order, which was alphabetical order by UDMF property name. That led to strange effects like basic properties being somewhere in the middle of the list (because "arg0" comes alphabetically way before "lineid"), side props being sorted by property rather than upper/middle/lower, and "class 10" coming before "class 2". Configuration order isn't necessarily ideal either, but from a quick glance, this looks to be a significant improvement.
I was already getting tooltips, but they were nonsensical. This is useful information not already exposed in the UI. Also opens the door to actual helpful tooltips later, maybe?
It uses log::debug, which apparently only applies when SLADE is run with a -debug flag, but the intention of LOG_DEBUG was for temporary stuff during development regardless of flags. I added SLADE_DEBUG before realizing the actual problem, but it's more correct than checking for NDEBUG anyway.
These are all probably my fault.
This allows keyboard navigation and, shortly, some other things.
The properties side panel looks up all the known UDMF properties on, say, the current line. This includes faux side properties like side2.sector. If a line actually has a back side, it'll defer that lookup to it; otherwise it'll treat 'side2.sector' like a real property name and fall back to MapObject::intProperty. That calls Configuration::getUDMFProperty, which looks the name up in a map. But looking up a missing key in a std::map creates a new value with the default constructor. As a result, mousing over a one-sided line would add several dozen blank UDMFPropertys to the configuration, all named after prefixed side properties. The visual effect was a huge stack of blank categories in the sidebar, manifesting as a sea of medium gray. Apparently wxPropertyGrid gets a bit confused with empty names and fails to return the existing group each time. Anyway, the fundamental solution is for getUDMFProperty to never create new canonical properties in the first place. It can already return nullptr, and every caller appears to check for it anyway, so this should be a safe change.
I recently ported a bunch of DECORATE objects to ZScript, and their sprites stopped appearing in SLADE. Some investigation revealed that the problem was that I used "//$NotAngled" as the last magic comment in many of them, and because the tokenizer tries to handle this itself, it would dutifully always consume the next token -- which was "states" on the following line. I couldn't figure out a way to fix this within the tokenizer, since it fundamentally can't know what it's supposed to be looking for. Even if it did know the syntax of every magic comment, the same problem would happen if someone put "//$Sprite" on its own line before a states block. That's incorrect, of course, but it's within a comment, so it shouldn't affect parsing of the rest of the file. So I've changed it to do a little sub-parse of the comment instead, and shared that code with the DECORATE parser as well. I ended up not using strutil::split(..., true), but there's a free feature, I guess.
Inspired by realizing that this whole time I've been convinced the standard Doom door delay is 50 tics. It's 105. Wait, no, that's also wrong, it's 150. Also found out why the combo box was weirdly tall on GTK: it's because the slider is weirdly tall, and the combo box is stretching to match it.
By "delete" I mean "reset to default values", since there's no real notion of a missing property. But it's handy for, say, nuking the special + args off of a line. This doesn't delete custom properties, either, because that breaks things; currently the list of custom properties persists until the whole grid is rebuilt.
This is just mildly annoying because my text editor highlights it in bright solid red :)
They're written out so neatly, grouped by function, and then the actual dialog just lists them in numeric order. This is much better.
We had both "Floors > Move Up" and "Floors Up", and "Doors > Remote" and "Remote Doors".
This is pretty jank in a lot of ways, but the old version was, too, and I don't think this is any worse. The keyboard navigation in particular leaves a lot to be desired. (Simply pressing up/down in the tree doesn't even work right? That's definitely not my fault.) Anyway, now you can do all sorts of things like filter the vanilla special list down to just S1, or find the locked doors by typing "red", or find the ZDoom portal specials even though you could make a good argument for finding them in Teleport or Rendering or even Sector.
I don't know why, but something about the order of events here produced a GTK warning and prevented the inner panels from being added to the tab control, so the dialog was just two big blank tabs. It shouldn't matter what the initial parent is since we change it immediately, and parenting to the dialog itself works fine.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm trying to map again, so I'm here fixing minor annoyances again. As always, C++ is not my native language and my dev machine is Linux, so if I made some poor architectural choices or broke something on Windows... sorry :)
Commits try to explain themselves, but the highlights are:
When a directory changes, make the resulting prompt dialog modal to the active window. Tabbing back to the map editor only to have the focus stolen by the main window for the sake of a modal, when the whole app is frozen under it anyway, is very weird.
Fix the splash screen repaint on GTK. Sort of. Not during startup, but at least when opening an archive or map. It also shows attached to the main window most of the time, now, which avoids giving it a taskbar entry (at least on GTK).
Fix the duplicate wheel events on the map canvas on GTK. This seems to be a wxGTK bug, but the duplicate events have identical timestamps, so weeding them out is very easy.
Fix rendering of texture offsets with negative scale. Seems like ZDoom ignores the sign when scaling the offsets.
Fix an infinite loop in mergeArch. I'm good at stumbling into these, apparently. In this case it resulted from precision cruft sneaking in after using Edit Objects.
Fix the 2D view not updating correctly after deleting a sector. This swaps the last sector into the deleted sector's place in the list, but because no geometry actually changed, the VBOs aren't updated, leading to some confusing between the new and old sector.
Remember configuration order for specials and UDMF properties. The config lists them in a human-curated and pleasant order, which was then being thrown away for the default order you get out of
std::map. The special tab and prop grid are much nicer to scroll through now.Show the names of UDMF properties in prop grid tooltips. This was really more about investigating why UDMF props were in the order they were in the first place, but it seems good to expose.
Spice up the props grid, just a bit. You can now click on the label column to focus it (which is oddly not default behavior), allowing for ease of keyboard navigation. You can also multi-select properties, which currently has only one use: pressing delete to clear them all at once. It could be nice to be able to copy a specific set of properties between objects sometime, though.
Fix nonexistent UDMF props being created when looked up. This led to every
side2.Xpseudo-property being created as a blank entry on lines, which in turn put a bunch of blank rows at the bottom of the line propgrid and confused the heck out of me. I guessstd::mapjust creates default objects if you so much as glance at it.Consolidate some group names for Boom specials. Boom had "Floor Down" and "Remote Door" groups, but Doom had these as "Floors > Move Down" and "Doors > Remote".
Fix the "Change Special" dialog being blank on GTK. I do not know what happened here but now it works so let's not question it.
Allow searching action specials by name. There's a big ol' textbox up there that only takes numbers! Now if you type text, it filters the list of specials down to those that contain what you typed.
There's not actually a filtering mechanism on the TreeView, so it has to nuke and repopulate the tree while you're typing, but there are only so many specials so it's... fine. It's fine.
Also, the selection and args panel should no longer get out of sync.
Add arg types for time. Now doors with a "delay" arg have a dropdown telling you that the standard door delay is 105, which is good for me personally, because for some reason I was absolutely convinced it was 50.
Fix some parse bugs with magic DECORATE/ZScript comments. It was possible for the tokenizer to eat real tokens from subsequent lines in the name of finding arguments for these comments, which would then confuse the rest of the parse. I couldn't think of a way to fix this within the tokenizer, so I changed it to sub-parse the comments.
Also some
LOG_DEBUGstuff. It just wasn't working, and then I found out it only works with the-debugflag, but it already compiles to nothing unless you're running a debug build anyway, so I fixed this, and also deleted some existingLOG_DEBUGs that I left lying around because they probably shouldn't end up in the codebase.