Generalize solid-object scripts #291
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
This script has very general application, but when it's in solid/rocks/big_rock, it feels limited to just this one object. Moving and renaming makes the general usage obvious.
Same deal as big_rock.gd--there's nothing log-specific about it, so let's make it obviously available for everything to use.
DRYing this out. This inheritance-based design only makes sense when you consider that different objects will have different mirroring behaviors. For a complex hierarchy of offset Sprite2Ds, for example, you'd need different code than for a CollisionPolygon2D. Thus, there's not much reuse to be gained from splitting Mirrorable and SolidObject into separate base classes.
Koopa1018
requested review from
GTcreyon,
AidenBeresford,
jaschutte,
Kuma-Boo and
Rowan-H
...not just Sprite. Is this solution overengineered? Absolutely. Efficient? Not likely. But it's hands-off and it should work for pretty much any set of child nodes you'd need for a solid object: sprites static or animated, colliders of any shape, and of course polygons. Hopefully it all works and I don't have to scale back.
It's a pretty awkward fix, but I don't have the brainpower today to correctly implement the static-dictionary-based method I prefer (partly because I think it may be best to clear the dictionary on scene transitions? And I don't remember the machinery we use for that).
gdscript y u no boolean xors
|
Notes from Discord:
|
I forgot that iterating an array of vectors yields copies of vectors, not references. So every element was getting copied, the copy was being written to, and the loop was moving on with the original unchanged. Mirrored rocks now have correct collision. Good that setter fns run when the game starts, even if they don't in the editor; the LD is still broken in this branch, so I had to manually add the rocks to the test level 😝
In this case, where it's already a big chain of elifs, it seems kind of silly not to do the same thing for this first one. Also add a comment to make it clear where ifs are NOT meant to segue into elifs yet.
Once the duplicate's been created, there's no point to doing it again.
This is one of those spots which looks a little arcane (at least when you're sleepy). Comments usually help with that at least a little.
Disabling by layer mask relies on setting the correct layer mask, which introduces an extra point of failure (the object's layer mask can mismatch what's coded). Godot's built-in disable-physics-object system does not have this limitation, but it does have multiple available disablement behaviors. In this case, though, the most common option is the default, and unlike layer masks, only needs to be changed if the other behaviors are needed. Even so, document the dependence on disable_mode in the class's docstring.
GTcreyon
approved these changes
Mar 6, 2024
Koopa1018
added a commit
that referenced
this pull request
Mar 9, 2024
...with one based on Godot's Process Mode system. (Same one used in #291 actually--see that PR for details.)
Koopa1018
added a commit
that referenced
this pull request
Mar 9, 2024
To solve merge conflicts resulting from merging #291.
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
Renames
log.gdtosolid_object.gd, andbig_rock.gdtosolid_object_mirrorable.gd. Also moves both of these scripts directly intoclasses/solidout of their subfolders. Neither of these scripts actually has anything specific to rocks or logs--one lets the LD disable a StaticBody2D, the other lets the LD disable and mirror a StaticBody2D. This seems useful for many different solid object types.Issue(s)
Minor issue noted during #286.