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

fix: editor reset button share same reference between objects #36656

Closed

Conversation

ThakeeNathees
Copy link
Contributor

when reset button at the Inspector dock pressed and if the default_value (at ClassDB::default_value) Type is OBJECT, not null -> the resource is Referenced by the property. It leads multiple nodes which have been resetted have the same reference, also After the reset button pressed first time ever the property has the reference of resetting resource -> so no more resets

Fix: #36372
Fix: #36650

@KoBeWi
Copy link
Member

KoBeWi commented Feb 28, 2020

There's another PR that fixes #36372, but in a completely different way: #36644

editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
@ThakeeNathees
Copy link
Contributor Author

I've no idea why my fix is fail building on Travis-CI. Could someone explain?
@Xrayez @KoBeWi

@KoBeWi
Copy link
Member

KoBeWi commented Feb 29, 2020

You need to use clang-format.

@ThakeeNathees
Copy link
Contributor Author

I've fixed formatting issue and still it fails building. What am I doing wrong?

@volzhs
Copy link
Contributor

volzhs commented Feb 29, 2020

CI failure seems not related to your PR.
don't worry about it. 😄

core/resource.h Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

You can click failing builds to see the build logs and the errors: https://travis-ci.org/godotengine/godot/builds/656630454?utm_source=github_status&utm_medium=notification

@akien-mga
Copy link
Member

CI failure seems not related to your PR.
don't worry about it. smile

It is related to that PR, otherwise it would happen in the master branch too. The errors are likely due to the invalid addition to object.h of a virtual method which is only overridden in Resource. It's unnecessary in the first place, so removing it should fix CI.

@volzhs
Copy link
Contributor

volzhs commented Feb 29, 2020

CI failure seems not related to your PR.
don't worry about it. smile

It is related to that PR, otherwise it would happen in the master branch too. The errors are likely due to the invalid addition to object.h of a virtual method which is only overridden in Resource. It's unnecessary in the first place, so removing it should fix CI.

oh. I need to keep in mind.

@kimjiy9607
Copy link

Hi,
I am doing a project for my class and found this pr.
I just want to learn more specific details so that I understand better.
I am still confused with what passing reference is doing to this scenario and in this case what the "property" means. Also, what happens if we say res->duplicate(true)?
Any help will be appreciated:)

Thank you so much!!

@ThakeeNathees
Copy link
Contributor Author

@kimjiy9607 the scenario here is if the reset button pressed for 2 (or more) path curves, they all will reset to the same resource (all have the same reference), which leads editing one curve reflect on the other, and here the "property" is a StringName representing the name of the property to reset,
res->duplicate(true) will perform a deep copy (instead of referencing it).

@akien-mga
Copy link
Member

Needs a rebase to squash commits into one, and make the commit log a bit more explicit.

@ThakeeNathees ThakeeNathees changed the title Reset button bug fixed fix: editor reset button share same reference between objects Jun 9, 2020
@ThakeeNathees
Copy link
Contributor Author

@akien-mga done rebase.

( btw it was my very first pr to godot )

akien-mga added a commit to akien-mga/godot that referenced this pull request Jun 9, 2020
To do so, Object gets a new virtual `object_duplicate` (`duplicate`
already taken by incompatible implementations in derived classes)
which is implemented in `Node` and `Resource`.

This allows fixing godotengine#36372 properly in `ClassDB` by duplicating
Objects that need it to avoid using the same one as default value.
(So far it seems only Path2D/Path3D's `curve` property uses this.)

Fixes godotengine#36372.
Fixes godotengine#36650.

Supersedes godotengine#36644 and godotengine#36656.
@akien-mga
Copy link
Member

Superseded by #39479. I added you as co-author as in the end I mostly reused the same fix as you found, just higher up the call stack.

@akien-mga akien-mga added this to the 4.0 milestone Jun 12, 2020
akien-mga added a commit to akien-mga/godot that referenced this pull request Jun 12, 2020
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>
@ThakeeNathees ThakeeNathees deleted the reset-button-fix branch July 2, 2020 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants