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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ProjectileEffect base class and basic functionality #2
Add ProjectileEffect base class and basic functionality #2
Changes from all commits
3880288
ccca142
756ecd3
6090e51
0c1217e
f08db8e
82f3f7f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Non-blocking: can we type check the effect? I guess we're loading the scene, but we could probably type check the parent node of the scene to be able to check against a custom
class_name
? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍷 🧀
This is not actually needed with the current implementation.
It WILL be needed if we want to use the Node class type instead of a PackedScene for effects management. I.e. you can't do
HugeLaser.new()
unless you define the class_name in a script.But if you load a
PackedScene
from the scene path, this is totally superfluous boilerplate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍷 🧀
I'm pretty sure the resulting type here is "Packed Scene", which currently cannot be typed better
I flip-flopped between the actual node type (ProjectileEffect) and PackedScenes and some other ideas so many times that changing every type annotation was incredibly cumbersome, so I've opted to leave this as just
Array
until we flesh things out and settle on something that works and won't be changed.There's also no support for Type Variables -- I figured having a Globals script that contains this type so we can just say
var EffectType = Array[PackedScene]
and use that so that making a change would be trivial, but can't find a way to do it with the current gdscript version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 Some interesting thoughts in this forum post on accessing more info about types when using PackedScene. A custom resource type is an interesting approach. Nothing for now though, just food for collective thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Classic me. No link, and now I can't find it again 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍷 🧀
This is a hack to allow the "scale" you set on the Projectile node itself to impact the actual scale of the base projectile.
Without this, setting "Transform -> Scale" on the base projectile node will change the size of the projectile in the 2D visual UI, but at runtime it will be completely ignored. See this godot pr, also linked in the commit message directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I kinda like the idea that projectiles can hit themselves / the player... but that'll take some thinking. For now 👍 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a terrible idea, though the huge double lasers overlapped hitboxes, so they would just immediately disappear