Octahedral normal/tangent compression corrupts meshes if upgrading a project from an older 4.0 alpha

[TokisanGames]

Godot version

d0a2a4c

System information

Windows 11/64, RTX 3070, Vulkan

Issue description

Related to the first time this happened: #55633 and #58592

Caused by #60309 @The-O-King @akien-mga

This model has blend shapes. But it happens to all of my meshes, whether they have blend shapes or not. I have my meshes saved in the scene files, but opening up an inherited scene from the glb looks the same.

Latest d0a2a4c and 7b4927b show this:

Previous commit acd8fb7 shows this:

Steps to reproduce

1. Open a mesh.
2. Cry.

Minimal reproduction project

Here is a mesh (with blend shapes though not required) imported in Alpha 14 that is now messed up in 7b4927b and beyond.

https://we.tl/t-PoFcVmDk7e (expires in 1 week)

[Calinou]

@tinmanjuggernaut Please upload a minimal reproduction project to make this easier to troubleshoot.

[fire]

I'll do a test on https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/MorphStressTest and report back.

Cannot reproduce with e172b1a

MorphStressTest.zip

@lyuma mentioned if you can reimport by deleting the .godot folder

[TokisanGames]

If I delete .godot/Dorian...glb I can reimport the mesh and it looks normal. However all of my existing meshes are messed up.

Please upload a [minimal reproduction project]

I added an MRP.

@fire Are you on the right ticket?

[fire]

I tested

with e172b1a and had this result after taking the data from the test project, exporting as glb and opening again.

If you broke the association between the scn and the original resource, it won't update.

[TokisanGames]

I tested x with e172b1a and had this result

Thanks for testing. She looks better, but shouldn't have her head split on top.

If you broke the association between the scn and the original resource, it won't update.

Yes, we do that on all of our hundreds of meshes. The engine usability pushes people to dissociate quite commonly, and storing glbs, or binary meshes and materials in git is a poor practice and can result in redundant data in git. My team already maxed out one repo and we had to purge and migrate to another repo because of it. So we made a conscious choice to save every resource except textures in godot's native text format.

If the arraymesh format has changed, then Godot needs to update the mesh data that is stored in its native format, not just force users to reimport hundreds of meshes any time that section of the engine changes.

[fire]

If the arraymesh format has changed, then Godot needs to update the mesh data that is stored in its native format, not just force users to reimport hundreds of meshes any time that section of the engine changes.

Do you think we should have this policy now or at beta?

[fire]

The previous policy was that only the original formats are expected to import into godot engine, and that in godot 4 release the mesh definition / internal formats would have upwards migration capability.

[TokisanGames]

The previous policy was that only the original formats are expected to import into godot engine, and that in godot 4 release the mesh definition / internal formats would have upwards migration capability.
Do you think we should have this policy now or at beta?

Thanks for asking my opinion. Short answer: Yes, now.

Since you provide internal formats, they should be fully supported. Right? I can't think of any good reason why you wouldn't support your own formats. They should always have an automatic upgrade path (with a warning if not backwards compatible). I should never have to reimport an asset just because the engine changed. If I want to reimport to take advantage of a new feature or change settings, that's different. But my old imported assets (whether inherited or not) should never break or should be upgraded automatically.

As for beta vs alpha, yes, I would begin this principle today. You guys need early adopters to test, especially those with large projects. There is no beta release without alphas being hammered on by early adopters.

Breaking changes are fine in alpha, but the general principle should be to provide an automatic upgrade for internal formats on each step, rather than require alpha testers to recreate their project every few months. It's not fair to us who are working hard to convert our projects without reliable conversion tools, amongst crashes and bugs all over the place.

We're helping to identify issues for you guys so late adopters will have a solid experience, and understand the consequences. But it's unreasonable to ask us to redo massive amounts of work like reimporting hundreds of assets multiple times because someone changed a format and didn't put in any upgrade code. And its ironic that the code is already in the engine, as you just demonstrated. It's just limited to the export and import classes, when it should be built into ArrayMesh or some asset version management class.

Being alpha isn't a good reason. Having upgrade code should be a required part of any PR that changes an internal format before acceptance starting today. And the PRs that are responsible for making changes demonstrated in this issue and my original issue for GD3 to GD4 changes #63550 should have conversion code added. I still haven't even finished converting over my assets, hoping you guys will include the conversion code. But if not, I am preparing to spend several more weeks manually reimporting hundreds of assets again. :/

[The-O-King]

Hello! This may be a naïve question but I did write some code to convert the normals/tangents from a Godot 3.x project to 4.0 - does that code also get run on projects that are upgrading through alpha versions of 4.0? I realize I did not write any code that converts from what was the old 4.0 format to the new 4.0 format but I can get that in pretty soon - but I wanted to see if that conversion code even gets called when going from different (I guess minor versions) of 4.0

Specifically I'm thinking of the code at 78881b3#diff-e84616c8a1b4f2029ec6cc009fb3ed17f43d699e1f9125d8023c5ce9b65e66a8 mesh.cpp:957 where I can check for the type of the mesh format (will I be able to tell if it's a 4.x vs 3.x array mesh?)

[Calinou]

does that code also get run on projects that are upgrading through alpha versions of 4.0?

No, it doesn't. It's only run once when the user asks to convert a project from Godot 3 to 4.

[djmaesen]

im having the same issue in alpha 15 ,

[YuriSizov]

The engine usability pushes people to dissociate quite commonly, and storing glbs, or binary meshes and materials in git is a poor practice and can result in redundant data in git.

The scenario you describe there is of your own making, from what I can deduce. If you commit glbs or other binary sources, you will have the exactly same experience as you do now by forcing them into text format. Except, they will still be importable and you will avoid the file association problem here.

You still commit megabytes of data to git every time you change the asset, probably even more so with text.

Your linked complain talks about binary files changed by Godot, but the only binary files controlled by Godot that come to mind are in the import folder. Which you are not supposed to commit. Godot would not change your glbs and other binary sources. So if that's the case, this is a scenario of your own making.

Your project your choice, but painting it as the engine forcing you down a broken path is very much unfair if you break out of best practices and do your own thing for your own reasons. I hope you can improve your workflow.

[Zireael07]

@YuriSizov: It's waay too easy to "break out of best practices" (which don't seem to be documented btw). For instance, meshes are saved as text if you press make unique to scene, with no indication that it will be so to a beginner user (people will only notice this if they use version control)

What he means by saying

binary meshes and materials in git [...] can result in redundant adata

is that git doesn't track changes to binary stuff, resulting in any change essentially reuploading whole file, instead of just the changes made

There is essentially no "best practice" here, as Godot seems to encourage having binary resources, but those are less than optimal for git. (Tbh I think @tinmanjuggernaut should be using LFS or some other method of file storage, not git itself, considering how large his proiject seems to be)

[YuriSizov]

is that git doesn't track changes to binary stuff, resulting in any change essentially reuploading whole file, instead of just the changes made

This is still true for their workflow, it's just that the diff is hidden inside of a scene/master resource file, with a giant serialized binary string.

From what I've read, their workflow issues start from a goal to avoid storing binary sources, which is indeed problematic with any VCS. LFS helps somewhat, but it's still a lot of data. But then they talk about binary files having changes done by Godot, and that can't be about GLB or other binary sources. So I conclude they instead commit and track the .import/.godot/imported folder. Which is very much not recommended and is disabled by our default .gitignore.

[fire]

@YuriSizov as far as I interpret this, it means they through a roundabout way, take the .godot/imported/ABCD.scn and save it into the vcs as text scene.

Edited:

This is an alternative to storing glb/gltf and letting godot engine convert as needed and through upgrades to the gltf importer.

[clayjohn]

I have a proposal to address this issue. Maybe too late for some, but ideally we can get a fix in place for others.

Basically we have no versioning for ArrayMesh formats, which is what leads to this pain every time we change the internal format.

To do a rough upgrade we look at how the naming of surfaces has changed. code here

If the name begins with "surface_" we know it is a 4.x surface and we can just import normally (that's a bad assumption I know). If the name begins with "surfaces" then it is either a 2.x or 3.x mesh (we have no way of knowing if it is a 3.x mesh before or after octahedral compression was added though).

I think we can add some very rudimentary versioning by doing the following:

1. automatically append "v2_" to "surface_" when saving arrays created in Alpha 16+ (i.e. "surface_" becomes "surface_v2_"
2. add compatibility code for surfaces created without "v2_" (convert old format to octahedral when loading)

This will allow upgrading alpha 14- projects automatically, but will still break with projects made in alpha 15 because there is no way to distinguish between an alpha 15 and alpha 14- mesh. The other downside is it is going to look pretty gross when the tscn is read as a text file.

Overall, I'm not sure we have any better options and we need to do better to avoid breaking projects during release cycles

[akien-mga]

How about adding a mesh_format_version without changing the surface* properties?

This can be added now for future changes (any compat breakage that requires a reimport/conversion should bump that integer, and compat code can be added).

For past, version less meshes, we can keep the current heuristics to detect 3.x meshes I guess. For earlier alphas we're in a tough spot indeed as we either break compat with pre alpha 14 or with alpha 15.

Is there any way that we could detect the list of meshes which may require conversion and ask the user to make a decision for each/all?

Alternatively, could we expose this as an editor tool that users like tinman can use manually to convert all their meshes?

[Zireael07]

Yikes. For actual beta, I think it would be ok to break pre alpha 14 (AND display a warning telling user to reimport assets), and keep compat with a15 (if someone was using alphas at all, I think it's more likely s/he was on the most recent one)

[MeshVoid]

Is it the same thing happening here with shaders made in GD4a14, and then opened in GD4a15?

[Zireael07]

Possibly, since if your meshes change, the shaders obviously will look different too

[TokisanGames]

Briefly:

• I would defer to Engine devs about best practices regarding C++ dev without question. However in regards to game development, unless any of you are actually building a game in a distributed team with git, I suggest none of you know any better than I or other game devs do. I've been managing a team of 5-10 people building a game for over 2 years. I know our needs. Suggested godot gamedev "best practices" are only ideas that are untested. We are testing them. Some suggestions by engine devs regarding our workflow are really good. The ones regarding mesh workflow have been poor choices. My comments on git and text resources are what we have found to be best for us, but they are only background information on this ticket. I'll skip discussing all of those comments as they didn't fully understand our workflow, eg. we do use lfs, and do not add .import/, .godot/ into git or even look in those folders.

• The whole point of this ticket and ArrayMeshes converted from Godot 3.x display incorrectly in 4.0 #63550 is that engine devs have not been supporting the native ArrayMesh format and have broken it multiple times, even on stable releases. By policy it should be a first class citizen (like FBX and GLTF are). Any changes to the format in a PR should require upgrade code to update older meshes.

@clayjohn's and @akien-mga's suggestions of ArrayMesh versioning are excellent. The other part is an official supporting policy, and we're on the right track.

The other downside is it is going to look pretty gross when the tscn is read as a text file.

ArrayMesh already looks ugly in tscn or tres. We have one tscn file as large as 350mb, and git or search and replace in notepad++ can process it just fine. Even the new --3-to-4 conversion tool will process it now, just no arraymesh upgrade.

Is there any way that we could detect the list of meshes which may require conversion and ask the user to make a decision for each/all?

When I open a godot 3 project in godot 4, it asks if I want to upgrade the project. That is implicit permission to upgrade the meshes. Or you could ask upon opening each mesh, but I think it will be redundant.

For past, version less meshes, we can keep the current heuristics to detect 3.x meshes I guess. For earlier alphas we're in a tough spot indeed as we either break compat with pre alpha 14 or with alpha 15.

3.x meshes really need a conversion. I'd say thats a must do. I have close to 1000 that are messed up. I'm sure other devs are in a similar situation. #63550

Alpha1-14 meshes it's a toss up. For me individually, I've only reimported about 20 animated meshes and 25 static meshes. I can redo those again. However @djmaesen appears to have all of his assets in alpha 14, and who knows about other early adopters. There are already several other youtubers who have been building games and scenes in alpha, and who knows how many have disassociated their meshes. More likely some gd4 gamedevs don't even realize they have disassociated meshes because it's very easy to do without realizing it.

Perhaps you could indeed just ask us which version we're upgrading from on a large table dialog, like the fix dependencies window when we open a scene. Godot will certainly be cataloging all arraymeshes then and can identify anything without versioning as gd3, then we can manually select Alpha1-14 instead.

---

@djmaesen, If your scenes are still associated with glbs, you could delete your .godot/imported folder and that will force a reimport of your files. Perhaps that will fix your meshes.

[YuriSizov]

I'll skip discussing all of those comments as they didn't fully understand our workflow

We'd like to understand it though. Besides my personal opinions, there are also engine needs, and Remi would love to hear from you so that we understand better why you've decided to convert your binary files into embedded strings. He poked you on RC if that's preferable to discussing it here (which it probably is).

On a personal note, best practices aren't untested even if you don't see the reason in them. But again, your project your choices.

[TokisanGames]

I never get on rc, but I'll look later. Thanks for the heads up.

[TokisanGames]

@akien-mga Thanks for the discussion. I took three things away from Juan's comments:

1. Things like ArrayMesh should not break within 3.x or within 4.x.

well, things should not break without an upgrade
if you mean what happened to octahedral encoding, I think that should probably have needed automatic conversion
...
Godot 4 its fully expected that compatibility breaks everywhere, but it should not happen within the same major version

2. Don't upgrade projects from 3 to 4, or don't count on support if you do.

everything is broken pretty much, and we have limited resources to do the 3 -> 4 converter
So IMO if you start your project in GD3, its generally better to finish it there, there will be LTS

3. The focus is flexibility on the importer rather than flexibility in the editor.

the main idea of the import preview dialog is that you can customize the scene as much as possible there, and then just import something that is akin to what you need

I think 3 is a mistake. A feature rich importer is good, but limiting those features later on is not. And "good luck" to anyone upgrading from 3 to 4 :(.

But for the purposes of this ticket, 1 is important. It sounds like ArrayMesh versioning that you and @clayjohn suggested will help me and others, and is inline with Juan's ideas. I'm already mentally prepared to reimport hundreds of meshes again. My concern is doing all that work again, and then having it break yet again. This solution and policy eases my mind.

[djmaesen]

some meshes still have wrong tangent calculation.

this screenshot is taken from a clean project in alpha 16 (so no upgraded mesh scene from alpha 14), imported the .glb file assigned the material and the tangents appear incorrect on some places.

[Calinou]

@djmaesen Can you upload a .glb (ideally with the .blend source alongside) of the mesh in question here?

[djmaesen]

@Calinou
here u go
exploBarrel.zip

[djmaesen]

this doesnt seem to be fixed yet, some of my meshes still have tangent errors

[djmaesen]

this still isn't fixed in beta12. whats causing this?
my barrel mesh still has the errors, and some weapon models also.

[lyuma]

@djmaesen I see you sent in the barrel mesh above: Having more test meshes could be helpful.

Honestly, I would recommend filing a separate issue. Please include a simple Godot project with the scene already set up to reproduce the problem.

The original issue was about a different problem, related to mesh corruption on upgrade. But if this is happening for a newly imported mesh from beta 10 or newer, it is not caused by upgrade so it deserves to be a new issue.

[djmaesen]

sorry but i dont want to share my models im using. the barrel mesh should be good enough to debug the issue

[clayjohn]

@djmaesen As Lyuma said, the issue you are posting about is different than the OP. Please make a separate bug report

[lyuma]

Closing in lieu of #63550 which concerns godot 3 upgrade. We cannot support upgrades from alpha builds.