Godot: Texture UV gets messed up when importing from Blender

[akjulu]

Godot version:
Godot Engine v3.2.2.stable.official

OS/device including version:
OS: Windows 10
GPU: Nvidia GeForce RTX 2060 Super with driver version 26.21.14.4108
Issue appears using both GLES2 and GLES3

Issue description:
Model imported from Blender seems to have corrupted UV mapping on some polygons.

Steps to reproduce:
This is a model I have made in Blender to demonstrate my issue. It's the same quad duplicated multiple times to create a bigger square. On the left is the 512x512 texture I'm using and Blender's UV editor. Each quad has exactly the same UV mapping.

When I export the model as .dae using the Better Collada Exporter plugin and import it to Godot, it ends up looking like this.

So, some of the quads look like their UV mapping is corrupted in various ways, even though there shouldn't be any difference between them. I have tried altering the import settings of both the model and the texture, but they don't seem to have any effect on the problem. I have also tried importing the exported .dae back to Blender, and it displays correctly there, so the issue doesn't seem to be with the export plugin.

Minimal reproduction project:
UVProblem.zip

[Calinou]

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

Also, which Godot version are you using? When reporting issues, make sure to fill in the issue template instead of replacing it.

[akjulu]

I added the issue template and a minimal reproduction project to my original post.

[TheDuriel]

Try triangulating before export. Looks like its trying to guess the triangulation of your quads, and gets it wrong. Either on export or import.

[akjulu]

I have tried triangulating both through the setting in the export plugin and by manually splitting the quads into triangles, but it doesn't seem to help.

[TheDuriel]

Given that collada is an outdated and mostly deprecated format. Does this persist when using GLTF on the newest/or next beta version of Blender?

[akjulu]

Importing the model in glTF format fixed the issue. Thank you for the suggestion!
My problem is now solved, but this still seems to be a bug in Godot. Should I close the issue or leave it open?

[Hevedy]

We are here with the same issue and I'm using glTF 2.0
Exported from Blender the file seems fine the normals are fine plus in external editor it seems fine, just in Godot makes no sense.
https://www.dropbox.com/s/jwgkgt7f4g70j0f/Godot_v3.2.2-stable_win64_2020-08-07_21-38-13%20%281%29.png?dl=0
https://www.dropbox.com/s/8dg0uo05sy789q8/chrome_2020-08-07_21-40-27.jpg?dl=0
https://www.dropbox.com/s/shas8ofw3d8g373/MeshBug.7z?dl=0

[kubecz3k]

We have ported the game to 3.2.2 recently and we have an issue which probably is related. Most of the models which are little more complicated have at least one polygon with wrong uv mappings:


reproduction_asset.zip
cc @RevoluPowered

[Calinou]

I can confirm @kubecz3k's issue on Godot 3.2.2, 3.2.1 and 3.2. It works fine in 3.1.2 though. I'm bisecting the regression right now (it's between 3.2beta1 and 3.2beta2).

Edit: Bissecting done. The regression is caused by 218f38c7ecdea970a5e82a48e7782077be4fc248.

Minimal reproduction project with the asset included: test_uv_import.zip

[RevoluPowered]

The problem is simply expressed like this

changing is_equal_to in vector3 is a bad idea and not correct as this case below will fail:

the reason for this regression is because the uv comparison will fail, you are seeing a symptom of the uv2's being merged incorrectly at import which is broken by Vector3 being changed incorrectly.

Vector3 vec = Vector3(0,0,0.1f)+Vector3(0,0,0.2f)+Vector3(0,0,0.3f)
if(vec == Vector3(0,0,0.6f) )
{ 
// this will never run with that commit 218f38c.
}

[RevoluPowered]

@Xrayez can you possibly make a test for this in master?

[lawnjelly]

// this will never run with that commit 218f38c.

Surely this is correct behaviour? That's what float math does.

You would normally have another explicit function e.g. Vector3::is_equal_approx with an epsilon to deal with cases where this was intended? Overriding basic math operators in this way is asking for trouble, as people will expect them to do what they say.

[RevoluPowered]

// this will never run with that commit 218f38c.

Surely this is correct behaviour? That's what float math does.

You would normally have another explicit function e.g. Vector3::is_equal_approx with an epsilon to deal with cases where this was intended? Overriding basic math operators in this way is asking for trouble, as people will expect them to do what they say.

Good point, then the problem is possibly the other way around, the importer is assuming this is the case?

I honestly think the == should hide the float problems away, but you're right the 'float' should expose it's native type data rather than the corrected data, but the real issue we have now is that is this the ONLY place relying on this behaviour?

From a users point of view / more novice ones it might be better to 'hide' it, from a programmers point of view it's incorrect so they should be using is_equal_approx not equal_to

[lawnjelly]

Yes, imo the problem is in the importer if it is not explicitly using an epsilon. This is ubiquitous in merging floating point vertex data.

[RevoluPowered]

Yes, imo the problem is in the importer if it is not explicitly using an epsilon. This is ubiquitous in merging floating point vertex data.

I can make a PR later on and take a look for the importer bug

[RevoluPowered]

https://github.com/godotengine/godot/blob/00949f0c5fcc6a4f8382a4a97d5591fd9ec380f8/editor/import/editor_import_collada.cpp#L741

This is the only thing I can see in the code which would explain the vector3 change issue, IDK though. I will use the sample project and debug when I get some time tomorrow

[Hevedy]

What about the non collada format error as I posted and sent an example (?)

glTF 2.0 have the same issue https://github.com/godotengine/godot/issues/41090#issuecomment-670684594

[akien-mga]

I can make a PR later on and take a look for the importer bug

Bump :)

CC @aaronfranke too as apparently the issue was caused by 218f38c.

[Reousa]

@akien-mga Will we not be getting a fix in a 3.x version?

[akien-mga]

@Reousa The current development version is 4.0, so that's where it should be fixed first. Then the fix can be cherry-picked for the 3.2 branch.

[aaronfranke]

Did this bug exist before #18992 was merged? That PR changes Vector3's == to use approximate equality, and 218f38c7ecdea970a5e82a48e7782077be4fc248 changes Vector3's == back to exact equality. If the bug existed before #18992, then it's not really a regression from 218f38c7ecdea970a5e82a48e7782077be4fc248 though the bug still needs fixing. If it didn't, then between #18992 and 218f38c7ecdea970a5e82a48e7782077be4fc248, it's likely that someone (possibly me, though I doubt it) changed code that relied on == being approximate. Regardless, the fix is the same, but this could help narrow down the problem.

In a nutshell, we need to find out where in the code it's incorrectly using a == b and change it to a.is_equal_approx(b). @RevoluPowered You are 100% correct that in many cases you don't want to use ==. @Reousa This kind of fix is absolutely cherry-pick-able, so once it's fixed in master (4.0) then it can be fixed in 3.2 as well.

[Reousa]

@Reousa The current development version is 4.0, so that's where it should be fixed first. Then the fix can be cherry-picked for the 3.2 branch.

@Reousa This kind of fix is absolutely cherry-pick-able, so once it's fixed in master (4.0) then it can be fixed in 3.2 as well.

I see, thanks for clarifying! Any tips on where to start if I was to help fix this? Perhaps where the import code is located.

[aaronfranke]

@Reousa The first thing I would do is try building commits from the time when this broke, then see what changed. You can first test before #18992, and then if the problem doesn't exist there then you would have to bisect between #18992 and #32477 (192 days of commits), and I think the best way to figure out where it broke is to do a bisect but cherry-pick #32477 on each commit in the bisect and check if it works (since we want to find out what change relied on the behavior of #18992, which #32477 reverts).

[lawnjelly]

Don't look at this further, I've found the bug. It is due to another approximate equal change in #18992.

The test for being a duplicate vertex in the set uses the Vector3 < operator, which the PR above has changed to not be a < operator, but some approximate equal version. Changing it back to a < operator fixes the bug.

I can do a PR to fix this, but really we should consider reverting #18992 and other PRs doing approximate equal changes to basic operators. I'll discuss this with reduz later. The problem is that having had this change in for a year, there could be other bugs that are dependent on these changes, so we are in the worst of both worlds.

[reduz]

All approx equal operators need to be reverted, approx equal should only be a special function.

[lawnjelly]

All approx equal operators need to be reverted, approx equal should only be a special function.

@aaronfranke would you be able to do this (rather than me make a PR)? You know the changes made and it seems you are best placed for reverting.

[aaronfranke]

The < etc operators were the one place where I wasn't sure what the desired behavior was. The idea is that if the X value is tied except for floating point error, it will instead check Y, same with Y to Z.

@lawnjelly Why does a test for duplicate vertices use <? Wouldn't it make more sense to check for duplicate vertices using == since the point of the operator is to check if they are equal (e.g. duplicates)? == is not approximate in both 3.2 and master.

[lawnjelly]

@lawnjelly Why does a test for duplicate vertices use <? Wouldn't it make more sense to check for duplicate vertices using == since the point of the operator is to check if they are equal (e.g. duplicates)?

You might well ask. I have no idea, it is how set appears to work (never used it myself). :grin:

It probably would be better to check with equal operator, however the point still stands about the reverting.

[aaronfranke]

@lawnjelly I opened #41894, but I'm still highly suspicious of this import code in the first place, especially since 218f38c7ecdea970a5e82a48e7782077be4fc248 doesn't touch the < operator but somehow was bisected as where things stopped working. Can you please point out which specific files and lines are responsible for importing and checking the duplicate vertices?

[lawnjelly]

@lawnjelly I opened #41894, but I'm still highly suspicious of this import code in the first place, especially since 218f38c doesn't touch the < operator but somehow was bisected as where things stopped working. Can you please point out which specific files and lines are responsible for importing and checking the duplicate vertices?

Just try your PR, it fixes the import bug.

Probably because both the equals AND the < operator are used in the texture import. They both probably have to be working correctly for it to function right. I think the < operator was used in the set::find function if I remember correctly, but if you step through debugging an import you should see how it works. I'm really not in a position to explain how the importer works, I didn't write it.

[akien-mga]

Fixed by #41894.