Skip to content
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

Object: Add usage hint to instantiate Object properties in editor #39479

Merged

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jun 12, 2020

Edited after changing the approach used to fix the bug.

Fixes #36372 as Path2D/Path3D's curve property no longer uses a Curve
instance as default value, but instead it gets a (unique) default Curve
instance when created through the editor (CreateDialog).

ClassDB gets a sanity check to ensure that we don't do the same mistake
for other properties in the future, but instead use the dedicated
property usage hint.

Fixes #36372.
Fixes #36650.

Supersedes #36644 and #36656.

Fixes godotengine#36372 as Path2D/Path3D's `curve` property no longer uses a Curve
instance as default value, but instead it gets a (unique) default Curve
instance when created through the editor (CreateDialog).

ClassDB gets a sanity check to ensure that we don't do the same mistake
for other properties in the future, but instead use the dedicated
property usage hint.

Fixes godotengine#36372.
Fixes godotengine#36650.

Supersedes godotengine#36644 and godotengine#36656.

Co-authored-by: Thakee Nathees <thakeenathees@gmail.com>
Co-authored-by: simpuid <utkarsh.email@yahoo.com>
@akien-mga akien-mga force-pushed the classdb-default-property-unique branch from 5fbc91f to b3bc5aa Compare June 12, 2020 12:34
@akien-mga
Copy link
Member Author

There's still the option to consider it a bug for this curve property to use an instantiated resource, and thus we could merge #36644 as the proper fix (but losing the current feature that Path2D/Path3D's curve is auto-instantiated in the constructor).

After discussing with @reduz, we agreed that the better fix would be for Path2D/Path3D not to auto-instantiate their curve property, but to instead have a dedicated property usage flag that can be used for Object properties that should be auto-instantiated when created in the editor.

So I've implemented that.

@akien-mga akien-mga changed the title ClassDB: Duplicate default property value when it's a Resource Object: Add usage hint to instantiate Object properties in editor Jun 12, 2020
@akien-mga
Copy link
Member Author

I think this new solution is a good approach overall, and it opens the possibility to do the same for more properties, e.g. GPUParticles2D/GPUParticles3D's process_material (to default it to a ParticlesMaterial and thus have particles working out of the box in editor).

Some concerns though:

  • Are there other ways to add nodes in the editor apart from using CreateDialog?
  • Custom classes should work fine for the Node being added, but what about properties of custom types? These would likely fail I guess.
  • Do we break the least surprise principle if nodes added from the editor and from script will behave differently?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant