godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.59k stars 21.27k forks source link

resource_importer_scene.cpp does not copy lods during _rescale_importer_mesh #94397

Open obhi-d opened 4 months ago

obhi-d commented 4 months ago

Tested versions

System information

Windows 10

Issue description

Bug in _rescale_importer_mesh, it should copy lods from surfaces in line 475. I have custom code to put custom LODs in a mesh and they are completely ignored. I will probably patch the issue and make a pull request later.

Steps to reproduce

Requires custom code with custom LOD to test. But the bug is evident when you look at the code in _rescale_importer_mesh in line 475.

Minimal reproduction project (MRP)

Requires custom code, but looking at the code should make it evident.

AThousandShips commented 4 months ago

I have custom code to put custom LODs in a mesh and they are completely ignored. I will probably patch the issue and make a pull request later.

Are you sure this is not because of your custom code? Or incompatibility with it?

If it's not possible to trigger this condition with the existing engine code it's not necessarily a bug, unless the use is as it is designed to be used, so please provide some steps to accomplish this within the scope of the existing engine code. Or a plugin, etc., to show that this is indeed a bug and not user error or just a limitation that isn't normally encountered because the system isn't designed for this use

obhi-d commented 4 months ago

As I said, just looking at the code would make it evident. There is no method in ImporterMesh to retrieve the lods as a Dictionary.

AThousandShips commented 4 months ago

Okay, but as I said, it being evident doesn't mean it's broken. What standard way would you provide the LODs that are ignored? What changes have you made with custom code? Are there any standard ways that these LODs are provided that are lost?

Otherwise this needs to be discussed quite differently

obhi-d commented 4 months ago

I get your point. The lod generation step is after the rescale, and there is no standard way to get custom LOD in godot.

AThousandShips commented 4 months ago

I'd say the LODs shouldn't be preserved, and the issue is perhaps dead code and that the LODs should be just passed as a Dictionary() instead

But if you have a workflow for passing custom LODs and you want that to be supported that's something to make a proposal for IMO, to gauge if it is needed and if the workflow you are using is valid.

obhi-d commented 4 months ago

A rescale should not break LOD indices.

My custom code is a very custom extension (not the microsoft one, because thats based on node) on gltf 2.0 and works together with an export gltf extension I wrote for blender to export custom LODs to gltf 2.0 and import them in godot through gltf hooks (a mesh hook that again is a change in the import workflow for GLTFDocument). So thats a lot of changes and I am just using it for my personal project. I it is interesting for the community I can propose it.

Calinou commented 4 months ago

Does this happen to automatically generated LODs as well, or only custom LODs?

I agree custom LODs should be copied over still, although you'll need to adjust your distance thresholds manually. (When using custom LODs, you're specifying those distances yourself anyway.)

obhi-d commented 4 months ago

Its specific to custom LODs only because the lod generation step is called after rescale and fills the LOD dictionary correctly, replacing it in its entirety. Only when you want to carry over the LODs you fill ImporterMesh with add_surface does the lods fail to carry over correctly. Like I said, I made a lot of changes in the code to support custom LODs within a Surface, and skip lod generation, so the issue is not reproduced in the standard pipeline.