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 reimporting scene with default values selected #79907

Conversation

kdiduk
Copy link
Contributor

@kdiduk kdiduk commented Jul 25, 2023

This commit fixes #78140

When the scene was re-imported with non-default values of some settings, re-importing it again using default values for those settings didn't have the effect. For example, the ticket #78140 mentions this issue with "Root Type" setting, however the problem affected any setting.

The reason of the problem was that when handling the reimport, a wrong dictionary object of the settings was used.

@kdiduk kdiduk requested a review from a team as a code owner July 25, 2023 22:13
@kdiduk
Copy link
Contributor Author

kdiduk commented Jul 25, 2023

Although everything seems to be working now, I still doubt whether we need to remove the if-condition in this block:

if (root_type != "Node3D") {
Node *base_node = Object::cast_to<Node>(ClassDB::instantiate(root_type));
if (base_node) {
scene->replace_by(base_node);
scene->set_owner(nullptr);
memdelete(scene);
scene = base_node;
}
}

@kdiduk kdiduk changed the title [bugfix] Fix reimporting scene withh default values selected [bugfix] Fix reimporting scene with default values selected Jul 26, 2023
@YuriSizov YuriSizov changed the title [bugfix] Fix reimporting scene with default values selected Fix reimporting scene with default values selected Jul 26, 2023
@YuriSizov YuriSizov added this to the 4.2 milestone Jul 26, 2023
@fire
Copy link
Member

fire commented Jul 26, 2023

@aaronfranke is also working in this area from the gltf document

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, this works as expected, and the fix makes sense.

Although everything seems to be working now, I still doubt whether we need to remove the if-condition in this block:

I have already fixed this logic in PR #79774, feel free to give it a review.

This commit fixes godotengine#78140

When the scene was re-imported with non-default values of some settings, re-importing it again using default values for those settings didn't have the effect.

The problem was that when handling the reimport, a wrong dictionary of the settings was used.
@kdiduk kdiduk force-pushed the fix-advanced-scene-reimport-default-settings branch from 7a70a53 to 8b729e5 Compare July 28, 2023 14:58
@aaronfranke aaronfranke merged commit ba3fb66 into godotengine:master Aug 1, 2023
@aaronfranke
Copy link
Member

Thanks! I hit the merge button myself due to how simple the fix is and I tested it.

@akien-mga
Copy link
Member

For the record for future commits, we don't usually prefix commits with a type like "[bugfix]". The title starts with "Fix" and makes it clear that it's fixing a bug already.

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 2, 2023
@kdiduk
Copy link
Contributor Author

kdiduk commented Aug 4, 2023

Just in case, if there are more maintenance releases planned for 4.0, this PR can be cherry-picked to 4.0 as well, as the change is trivial, and the bug was originally reported for 4.0.3.

@YuriSizov
Copy link
Contributor

I think for something like that we'd suggest updating to 4.1.x instead.

@kdiduk kdiduk deleted the fix-advanced-scene-reimport-default-settings branch August 4, 2023 15:39
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

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

Successfully merging this pull request may close these issues.

Can't reset root node type to Node3D in advanced import settings
5 participants