Importer option for permanent scaling of the scene #60115

SaracenOne · 2022-04-10T20:15:07Z

This is a draft PR tied to a proposal to replace or provide an alternative to the existing importer root_scale option (godotengine/godot-proposals#4379). The root_scale option only provides a one off modification to the root node's scale value when its instantiated, whereas this option provides a permentant modification to the underlying geometry and animations of a scene.

Left in draft while proposal is considered and the PR is being throughly tested for bugs.

Thanks to @lyuma for the original script implementation of this which I used as reference when writing this C++ version.

editor/import/resource_importer_scene.cpp

fire · 2022-04-11T12:10:56Z

Typo on the commit message. permentant -> permanent

marstaik

I see a lot of unnecessary data copies, and a lack of const correctness. Can you go through and check on them? A lot of the data copies will not be able to be solved due to lack of move semantics however.

editor/import/resource_importer_scene.cpp

lyuma

Overall looking ok.

I do wonder... do we need to consider other objects types for rescaling? For example, light attenuation? camera scale/near/far?

lyuma · 2022-04-11T20:57:40Z

editor/import/resource_importer_scene.cpp

+			while (i < current_bs_vert_arr_len) {
+				current_bs_vertex_array.set(i, current_bs_vertex_array[i] * p_scale);
+				i += 1;
+			}
+			current_bsarr[ArrayMesh::ARRAY_VERTEX] = current_bs_vertex_array;


This code is really messy.

I did things this way in my GDScript implementation because we do not currently have access to a better API for modifying ImporterMesh.

But since we're making a core engine change, it might be better to provide access to the vertex positions of EditorSceneImporterMesh so that they can be modified without deleting and readding everything.

Here's or chance to improve the APIs to make them less clunky.

fire · 2022-05-10T12:31:51Z

Want to rebase and try resolving the review comments?

fire · 2022-06-12T02:31:25Z

According to @lyuma this resolves the blend shape bug. Will test.

TokageItLab · 2022-07-30T08:15:47Z

Can this function be applied to rotations as well? I was thinking of something similar in #63662. I prefer that those transformations need to be PreProcess for retargeting.

fire · 2022-07-30T08:20:21Z

I want to propose to set the root transform as the default value of this importer option.

SaracenOne · 2022-07-30T18:30:52Z

I'm opening this up for review again based on the changes implemented by @lyuma and @fire. I shall check it again though since I haven't touched it for a while

fire · 2022-08-07T08:37:49Z

I'll see if I can reset the branch to avoid the big diffs.

modules/gridmap/grid_map.cpp

editor/import/resource_importer_scene.cpp

reduz · 2022-09-05T11:49:31Z

Seems good for the most part, should be able to merge it

fire · 2022-09-05T15:30:45Z

Will revise the for loop.

fire · 2022-09-05T16:46:39Z

READY TO GO

scaled the blends shape test by 10 and then apply
check scaling on root and the animations play
done

MorphStressTest.zip

fire · 2022-09-05T16:57:53Z

Consider other objects types for rescaling? For example, light attenuation? camera scale/near/far?

Cameras and punctual lights are like points, so scaling a point doesn't do much. I don't know if scaling a light's light-units by distance-scale has a good formula since the physical light units is not default.

A camera is a node 3d, might work by chance.

tl;dr not a blocker.

Calinou · 2022-09-05T17:58:44Z

Cameras and punctual lights are like points, so scaling a point doesn't do much. I don't know if scaling a light's light-units by distance-scale has a good formula since the physical light units is not default.

Scaling a positional light by increasing its range should work well enough in some cases. Keep the same attenuation, and keep the same spot angle.

Cameras should be kept identical – don't change their FOV depending on scale.

fire · 2022-09-05T19:57:24Z

@reduz can you remove the merge block? The while loop has been removed.

Changes done.

akien-mga · 2022-09-06T11:38:33Z

Thanks!

SaracenOne added topic:editor topic:import topic:3d labels Apr 10, 2022

SaracenOne mentioned this pull request Apr 10, 2022

Replace root_scale import option with permanent modification to an imported scene's scale godotengine/godot-proposals#4379

Closed

Calinou added the enhancement label Apr 10, 2022

Calinou added this to the 4.0 milestone Apr 10, 2022

fire reviewed Apr 11, 2022

View reviewed changes

editor/import/resource_importer_scene.cpp Outdated Show resolved Hide resolved

SaracenOne changed the title ~~Importer option for permentant scaling of the scene~~ Importer option for permanent scaling of the scene Apr 11, 2022

marstaik reviewed Apr 11, 2022

View reviewed changes

lyuma reviewed Apr 11, 2022

View reviewed changes

SaracenOne force-pushed the scene_scale branch from 86d1a7e to 595f49a Compare June 9, 2022 15:34

lyuma force-pushed the scene_scale branch from 595f49a to f2bc626 Compare June 12, 2022 01:10

fire mentioned this pull request Jun 15, 2022

Vulkan: Incorrect glTF mesh rendering due to automatic LOD kicking in too early with scaled bones #57895

Closed

fire mentioned this pull request Jul 9, 2022

Warnings "AABB size is negative" when importing glTF file with zero-weight bones #56458

Closed

This was referenced Jul 29, 2022

Animation Retargeting Scale is .01 and corrupts the visuals #63641

Closed

Add apply node transforms option to rest fixer #63662

Merged

fire force-pushed the scene_scale branch from f2bc626 to a529dc9 Compare July 30, 2022 18:22

SaracenOne marked this pull request as ready for review July 30, 2022 18:25

SaracenOne requested review from a team as code owners July 30, 2022 18:25

fire force-pushed the scene_scale branch 4 times, most recently from 800fc97 to 6cb1fdc Compare July 31, 2022 14:08

fire force-pushed the scene_scale branch 8 times, most recently from 9c3bf07 to dcdedb8 Compare August 5, 2022 00:37

TokageItLab mentioned this pull request Aug 5, 2022

Add BonePicker and simple auto mapping to BoneMapper #63854

Merged

fire force-pushed the scene_scale branch from dcdedb8 to 19747ca Compare August 7, 2022 20:04

fire marked this pull request as ready for review August 7, 2022 22:56

akien-mga reviewed Aug 30, 2022

View reviewed changes

modules/gridmap/grid_map.cpp Outdated Show resolved Hide resolved

fire force-pushed the scene_scale branch from cdc2270 to a7660ed Compare August 30, 2022 17:04

akien-mga requested a review from reduz August 30, 2022 17:41

reduz previously requested changes Sep 5, 2022

View reviewed changes

editor/import/resource_importer_scene.cpp Outdated Show resolved Hide resolved

Importer option to apply the root basis for meshes/skins/animations

befc4b7

fire force-pushed the scene_scale branch from a7660ed to befc4b7 Compare September 5, 2022 16:46

akien-mga approved these changes Sep 6, 2022

View reviewed changes

akien-mga merged commit a49ec43 into godotengine:master Sep 6, 2022

TokageItLab mentioned this pull request Feb 7, 2023

Add an import setting to rotate the imported scene by 180 degrees #72753

Closed

lyuma deleted the scene_scale branch May 19, 2023 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Importer option for permanent scaling of the scene #60115

Importer option for permanent scaling of the scene #60115

SaracenOne commented Apr 10, 2022 •

edited

Loading

fire commented Apr 11, 2022

marstaik left a comment

lyuma left a comment

lyuma Apr 11, 2022

fire commented May 10, 2022

fire commented Jun 12, 2022

TokageItLab commented Jul 30, 2022

fire commented Jul 30, 2022

SaracenOne commented Jul 30, 2022 •

edited

Loading

fire commented Aug 7, 2022

reduz commented Sep 5, 2022

fire commented Sep 5, 2022

fire commented Sep 5, 2022 •

edited

Loading

fire commented Sep 5, 2022 •

edited

Loading

Calinou commented Sep 5, 2022 •

edited

Loading

fire commented Sep 5, 2022 •

edited

Loading

akien-mga commented Sep 6, 2022

Importer option for permanent scaling of the scene #60115

Importer option for permanent scaling of the scene #60115

Conversation

SaracenOne commented Apr 10, 2022 • edited Loading

fire commented Apr 11, 2022

marstaik left a comment

Choose a reason for hiding this comment

lyuma left a comment

Choose a reason for hiding this comment

lyuma Apr 11, 2022

Choose a reason for hiding this comment

fire commented May 10, 2022

fire commented Jun 12, 2022

TokageItLab commented Jul 30, 2022

fire commented Jul 30, 2022

SaracenOne commented Jul 30, 2022 • edited Loading

fire commented Aug 7, 2022

reduz commented Sep 5, 2022

fire commented Sep 5, 2022

fire commented Sep 5, 2022 • edited Loading

fire commented Sep 5, 2022 • edited Loading

Calinou commented Sep 5, 2022 • edited Loading

fire commented Sep 5, 2022 • edited Loading

akien-mga commented Sep 6, 2022

SaracenOne commented Apr 10, 2022 •

edited

Loading

SaracenOne commented Jul 30, 2022 •

edited

Loading

fire commented Sep 5, 2022 •

edited

Loading

fire commented Sep 5, 2022 •

edited

Loading

Calinou commented Sep 5, 2022 •

edited

Loading

fire commented Sep 5, 2022 •

edited

Loading