This repository was archived by the owner on May 13, 2025. It is now read-only.
Feature ld polygon editor - #299
Draft
jaschutte wants to merge 69 commits into
Draft
Conversation
I'm not happy with this, maybe we should split the moving of old nodes and placing of new ones into two seperate modes. Since right now it is very finnicky.
For easier editing in isolation.
Not great having them mixed in with everything else, especially when there's a folder for it now.
The original name annoys me because the fact that verts are buttons is an implementation detail. Calling code only needs to care about whether "vertices" are visible.
These names don't confuse; the word "should" just doesn't add information.
Godot docs say instantiating like this is actually faster than spawning them in code. Theoretically more maintainable too. I can't imagine the performance of this code is actually going to make a dent.... Not integrated into code yet.
NewVert, not AddVert!
This signal seems to have come detached in making the poly-editor its own scene?
Nothing seems to have broken. That's good.
This one is weirder...when I was testing, I found that recycling the same node over and over would cause confusing behavior when adding verts. The problem didn't occur when we were deleting and reconstructing the add button every time, so...just do that now, despite the premade node apparently being perfectly good as-is?
Simplify predict-line drawing into a single clause of an if statement, rather than being scattered across two different spots with normal-line drawing awkwardly rolled into one of them using inline if. Also, remove "transparent_connector," as it was perfectly synonymous with draw_predict_line and wasn't even respected during the second predict-line draw.
For starters, identify what this section of code even is. Then the usual comment, clarify names, de-square the constant's value for ease of editing, etc. Trying hard to match existing logic behavior. Unintuitively, "if mouse_position" is always true. If mouse position is ever null (is that possible?), the code which calculates nearest_position will crash before the code even reaches the if statement. In other words, nearest_position implies mouse_position, so it's valid logically to only check the former.
Based on the stackexchange post linked in the comment. Untested, but can confirm adding it to placed-vert didn't break anything.
Well that's interesting, This event still fires when I press LMB on a vertex, move mouse off, then release. Doesn't do that with vanilla buttons--moving mouse off the button always cancels the mouse click....
Now I've got the opposite problem: the button is almost never responding to anything....
Ah! Here we go. Everything works when it should, and doesn't when it shouldn't.
Doesn't call any code yet, but it's ready to, and that's a good start!
Decoupling is all well and good, but. Are we *really* ever going to use PolygonDraw except as a child of PolygonEditor? And if not, why add the extra indirection? In the process, refactor polygon_editor's function names to be more external-caller-friendly.
...taking pains to avoid changing logic. Lots of changes to support type-hinting, and encapsulate some polygon_draw functionality behind functions so polygon_draw is in charge of more of its own logic. (Add an assertion in the one place I didn't quite preserve logic--just to make sure we know if the weird edge case that triggers it ever happens?)
...to standard, and also for clarity and intuitiveness.
As the code comment says: it's not really doing anything to fix the terrain, and it's causing usability problems where vertex 0 separates into two when you click it (and it's easier doing this than figuring out how to keep two verts glued together in the editor).
...instead of a free-floating bool. This ensures state-exit code (which might e.g. ensure state-specific widgets get cleaned up) runs before playtesting begins. Incidentally, move the start button's state change call before any of the other logic, to ensure the state-change signal emits while the editor is in a fully valid state.
This reverts commit 0227714. I'm not convinced this was a good idea, and it certainly wasn't a good solution to the problem I'm trying to fix.
It feels like Main should be responsible for playtest start/end somehow? It kind of already is with the "ld_exit" check. These signals will make it much easier to keep poly-edit out of inconsistent states.
Before this commit, playtesting while polygon-editing would leave the edited polygon in a weird, invisible-but-still-solid state, and with its pre-edit geometry. No longer--pressing play now commits the terrain edits the same as pressing ld_poly_finish.
To parallel playtest_started in guaranteeing a fully valid LD UI. Actually--document that guarantee so calling code knows it can rely on it.
...to selection_handler rather than selection_action. It's got nothing to do with the action buttons. Also, the signal works fine as an editor-defined static connection; completely skip the callback add/remove logic.
I forgot Godot was annoying about treating statics as local fns 😒
Collaborator
|
With the current state, we have a polygon editor which is basically usable. Edge selection is going to be a whole new job's worth of work, and isn't essential for the immediate term. What say we get this merged as-is and handle the other two checkboxes later? |
Keeping this commit light, and there's STILL a bit to unpack! The gist of it is, placed verts now respond to left clicks on press, so it feels like dragging and dropping. Changed their mouse icon to make that clearer, too. Implementing this required a new script for placed verts, so that right click (to delete) could still respond to release only. (Because each button is now specially handled, the button action mode is now fully ignored.) Kind of awesome that I didn't have to modify polygon_editor or polygon_draw to end drag on mouse up--it just Already Did That.
It feels *wrong* somehow to have the move icon while the vertex is being deleted. Refactor fn to formally make room for handling other than main clicks.
Better than reverting to pointer state, I'd say.
Helps avoid visual clutter, I think.
Interesting that the body pattern shifts on finalizing edits. Which I think is because the origin is changed when the polygon is totally rebuilt.... We're not even planning to save polygon origins, are we? I think I can fix that by just originating all polygons at (0,0)....(or just at snapped intervals?) Includes an on/off switch just in case.
cruglet
force-pushed
the
feature-ld-polygon-editor
branch
from
August 20, 2024 00:41
234aecf to
8fcebf5
Compare
Simple enough, fixes the issue where only the first two vertices and the last vertex (EDIT: of the display polygon -@Koopa1018) would be the only ones to be rendered. Also, maybe we should make the terrain that is being modified slightly transparent? 🤔 Co-Authored-By: Cruglet <[email protected]>
Original author: @cruglet Original title: "Fix errors regarding out of index vertices/lines" Also fixes a related bug where the display polygon would crash when verts were added to the last edge, due to out-of-range indexing. Co-Authored-By: Cruglet <[email protected]>
Koopa1018
force-pushed
the
feature-ld-polygon-editor
branch
from
August 20, 2024 16:16
8fcebf5 to
b63905d
Compare
Cleanup today includes respacing and underscoring added functions to standard, as well as adding an error assertion to safeguard against unintended behavior in (hopefully) a clear, readable manner.
Added functionality to show whether a polygon is valid or not, by making the outline red if any self-intersections are found within the polygon. Also checks for stacked vertices on top of each other. Will revisit later.
Fixed issue where the selection handler would take priority and try to open the item properties menu for terrain. Also made sure that when the terrain is freed through the selection handler it is at least able to run drawable_polygon.end_edit before it exits the scene tree.
Self explanatory, if the terrain was valid before a new one was made then the state would carry over
Space all functions properly, fix single-line if statements, and put comments in more sensible spots. This seems like a good time to make the functions of PolygonValidator static, since PolygonValidator has no state anyway. Instancing a class fresh every time you need its functions is what I'd call code smell.
Having the first line in invalid red is quite alarming and quite unnecessary; and it is, in fact, possible to have >3 verts during vertex creation, meaning validation *does* need to be done. Since it's the same validation code as during dragging, let's just update both to use the same block of code.
Why highlight verts in invalid positions when you could just...not allow invalid positions? This took a lot of work to get working correctly 😆 (What about non-adjacent colocated verts? Simple. I believe those should not be considered invalid.)
...when the shape is completely flat, all verts colinear. In that case, all edges intersect with at least one other edge.
Or rather, first vertices of edges. Currently this is just used as a slower version of the same function, but my eventual plan is to use the extra data to highlight only the specific edges that intersect.
Terrain actually does need a property menu. Creating it is out of scope for this PR, but I'd rather have the crash than not--that way it's a constant reminder "implement the property menu, implement the..."
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Implements the polygon editor for the level designer. This PR aims to fully implement the polygon editor. This editor is supposed to work for all "things" which require a polygon to edit.
Todo list