Fix Level Designer bugs #286
Merged
Conversation
This file contains 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
Includes only the scripts found in the main editor scene's node hierarchy. Scripts were updated so that variables are in the correct order (export, private, onready); so that functions are in the correct order (internal fns like _ready, _process, etc. before any others); and so that functions are spaced to standard (2 lines between, no fewer).
Been a while since I've seen a true syntax error.
Item pane doesn't scroll far enough, and for now I'm looking to fix it _without_ switching to ScrollView (save widget replacement for a later PR). As always, starting by getting the script readable.
So it's clear what it's used for. (I guess I follow some self-documenting principles....)
I've had to modify the item grid before, and I'm tired of having to tease out *which* parent class populates it. Just move it into a node directly related to said grid. Script is now self-starting, instead of having main start it. I'm sure this can be done better, but this appears to work for now. This puts this branch squarely in the realm of refactoring.
Need to rewrite comments. These were mostly to get my head around everything. Leaving myself a note so I don't forget to go back and do this, as well.
...by assigning a class name.
Wish I could've saved this until things were fixed from Godot 4 upgrade, but alas...couldn't figure out the root cause.
Just giving it a basic gray box for now. I'm willing to do a widget swap for a bugfix, but reformatting the scrollbar assets to look right in the native system...that's a task for another day.
Item icons were stretching to fill the button. This led to ugly stretching artifacts, and items being larger than they were drawn. Gotta love when problems have simple solutions :)
I wondered, why not just use the built-in Icon field? This is why.
Also correct mistaken claim that item_textures only references item-pane icons.
Some comments were already in this one. I appreciate that. A few functions were renamed, but there's one outstanding rename change I'm waiting to make because it spans multiple files.
Now it can be used to type-hint. Which I've already used in the previous commit.
"Buffer" is quite ambiguous. Gotta be clear that it's a level we're talking about here!
I knew I was missing some when I hit every script in the scene tree.
Changed "key" to "typestring" so it's clear what the "key" actually is. Specificity over vagueness!
so I'm adding them to the list.
With reluctance, use the "x = x [operator] y" construction instead. (Sticking with || instead of "or" though--style guide notwithstanding, the word "or" is more confusing than clear in this context. It's not a condition--it's a literal Boolean operation!)
Old function scaled the mesh uniformly on each axis rather than adding an exact thickness to each side. The UV function didn't know this, causing terrible UV squishing as the outline grew thicker. The new algorithm doesn't exhibit this problem, and is also branchless. Also, kind of silly to recalculate the UV for every fragment. We're not using any per-fragment data in their calculations; let's just do that in the vertex shader and save the extra work. The new algorithm is deeply flawed, unfortunately--Shine Sprites break it in no fewer than two ways.
I don't fully understand the algorithm here, but it seems pretty simple. Also, thank goodness for TEXTURE_PIXEL_SIZE--it really does work! Four problems remain: - The first Shine Sprite icon in the pause menu is invisible with this shader, even if width is set to 0. - Sprite offset squishes/expands texture as it goes down/up, proportional to outline width. (An offset of 0,0 looks normal.) - Sprite offset slightly shifts texture along offset direction, also proportional to outline width. When Y offset = -1.5, each outline step moves the sprite 1/16 a pixel upwards. - When the outline is wide enough, bits of neighboring frames will show up on the edges of the bounding box. Some of these seem solvable. Probably all of them are and I'm just not brain enough to see that tonight.
Belongs with everything else, so it can be found without resorting to a search engine.
This function is actually called *by LDMain,* so I keep getting parser errors due to cyclic dependencies.
Now it explicitly handles items with no assigned textures, without even a defined property in the list. Also, assorted message improvements, and type hints for the sake of autocomplete.
This was definitely more work than necessary, and almost certainly not worth it. Still, it's done. Item texture dictionaries can no longer be null, and thus calling code no longer has to account for that possibility. (Before, they would have been null anytime an item had no texture property.) Unexpectedly, in combination with commit ebe80df, this seems to have fixed a bug where rotating blocks were not listed in the items menu. (And if that was intended behavior, well, now item_pane.gd has a clear place to include that :) ) Also, item_textures is now strongly typed, allowing autocompletion on its entries.
Now here's a nice, simple set of changes. The name "key" bugs me--it doesn't give any clues about what data is actually contained in the variable, which makes code difficult to understand. Thus the change to "propname" (given that fields' labels are being assigned its value, I'm inferring that that's what's in it--the property's name).
...by making their materials unique, then setting offsets appropriately. Shine icon in pause menu now appears again! (With...outer pixels extending past the boundaries. Ah well--it's a step up, at least.)
The extended pixels from the pause menu shine icon no longer show up. Some sides' outlines don't seem to realize that, and I'm not sure why.... Oddly, both of these problems ONLY affect the level info shine icon, and if you set cel count wrong, shine sprites themselves. I thought it was the combination of offset and single-cel sprites, but no....offset doesn't appear to affect shines that way.
Mark offset-related outline problems solved, and make notes on my current plight.
Ah, yes. Let's get that done so it doesn't have to be done again later.
Value should be the same, but now you don't have to parse hex in order to see the clear bits.
Koopa1018
added
bug
At this point, a similar check has already zeroed the alpha outside the cel--which means the one alpha check encodes all five of the OR conditions.
This fixes the bug where multi-selecting items with different sprite sheets causes terrible visual breakage. Been planning this for quite a while--maybe I should've done it the naive way first, I dunno. But it seems like I managed to get the memory-optimized\* version working with only one try, which I'm super proud of. \*not confirmed
This was referenced Jan 27, 2024
Everything useful has been moved out to other documents. With this gone, we can finally get merging probably.
Weird! It seems like it was starting the fade-out tween at basically silence. Bet that was because the fade-in was starting, applying its effect to volume, and essentially passing that changed volume to fade-out for it to use as its starting value. (The fact that it's storing a starting value *at all* is a prime example of a 4.1 change breaking things.) Setting fade-out's starting value explicitly fixes the bug.
|
@GTcreyon @jaschutte poke |
Sorry for the delay. I'll get on this soon. |
GTcreyon
suggested changes
Mar 5, 2024
Creyon confirmed that this script is not, in fact, needed in Godot 4.1. Nice.
Creyon said: "This can go. The planned feature has been implemented." Not really anything more to say.
...with one based on Godot's Process Mode system. (Same one used in #291 actually--see that PR for details.)
To solve merge conflicts resulting from merging #291.
Turns out that when I refactored backgrounds in #184, I removed a node which looked unnecessary, but which the LD was secretly using to darken the BG. As a result, since merging from master, the LD would crash immediately on game start. To fix the crash, let's use a plain translucent ColorRect in place of a code-based darkening system which relies on the BG's node structure. It looks identical, and this way, future BG designers don't have to worry about One Arcane Node That Can Break The Game If You Don't Include It.
|
Arright, that's all discussions resolved, merge issues resolved, and an extra bug caused by the merge fixed. Ready for merge now? |
GTcreyon
approved these changes
Apr 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of changes
Fixes most of the bugs I could find in the Level Designer after the 4.1 merge. Since the LD didn't run in build 0.1.6.alpha, I had no idea what was from 4.1 or not--so I just fixed everything I ran across.
Changes include:
To do before review:
As mentioned earlier, one bug remains in the glow shader...the pause menu's shine icon has an outline which extends too far on two sides.Going to deal with this separately, as Stray pixels in glowing pause menu shine #289.Out of scope for this PR:
Issue(s)
No GitHub issues, but these bugfixes are needed for LD work to begin in earnest.