search

vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
457 stars 72 forks source link

Added support for COLOR_n and TEXCOORD_n in morph targets - #116 #120

Closed acecebov closed 2 years ago

acecebov commented 2 years ago

Added support for COLOR_n and TEXCOORD_n in morph targets.

vpenades commented 2 years ago

Thanks! I'll try to review the PR this weekend

acecebov commented 2 years ago

Hi @vpenades, thanks for looking into this PR.

I am aware of breaking changes, just I thought it does not matter as it is still an alpha. I guess I can try do some changes to make it backwards compatible.

Will fix the tests as well. I run them once and everything was green, but then I added couple of more changes that finally outputted the extra target attributes, got me excited and forgot to run the tests again :).

Additionally there are no UnitTests that cover the new attributes. Should I add unit tests for that?

If you have any other concerns and suggestions, please let me know and I will do it.

acecebov commented 2 years ago

The API backward compatibility checks only with alpha-0011. The PR changes were made on top of alpha-0025. Where can I find API.Toolkit.1.0.0-alpha0025.txt and API.Core.1.0.0-alpha0025.txt?

vpenades commented 2 years ago

The API backward compatibility checks only with alpha-0011. The PR changes were made on top of alpha-0025. Where can I find API.Toolkit.1.0.0-alpha0025.txt and API.Core.1.0.0-alpha0025.txt?

They don't exist, I stopped tracking the changes after 11 because it was a lot of work on my side to maintain the diff tool. I am aware dotnet has some API diff tools to detect breaking changes between versions, but I never had the time to research and integrate them.

Anyway, the changes are very minor, and it might affect very few users, and as long as the tests pass, it's fine.

The latest changes are good. Give me some time to do a more throughful review just in case I missed something. In the meantime, it could be nice if you could write a unit test or a demo showcasing this feature.

vpenades commented 2 years ago

Done. @acecebov thanks for the contribution!

vpenades commented 2 years ago

@acecebov I've been doing some additional tests and I've found some issues that might prevent color morphing from working correctly.

Usually, the mesh packer tries to find the most appropiate encoding for every vertex element, in the case of colors, it uses a packed UINT32 to encode every RGBA component into a single byte.

As it happens, morphing channels are relative to the base value (hence the subtraction/addition tricks in the vertex types), so what happens if we subtract 255 from 0? yes, it goes into overrun because an unsigned bytes cannot represent negative values.

The only solution that comes to mind is to force the color format to be a full vector4 value when morph targets are detected.

AleksandarCebov commented 2 years ago

Interesting. So you want to force color format to be just like POSITIONS in morph target, right?

vpenades commented 2 years ago

yes, it would be a VECTOR4 instead of an UBYTE4

consider your example: RED (1, 0, 0, 1) morphed to GREEN (0 , 1, 0, 1) , the actual delta value stored in the morph target is Green - Red, which is (-1,1,0,0) , since -1 cannot be encoded into an unsigned byte, it produces an invalid value.

So the thing is, when packing the vertices, detect whether any primitive of the mesh is using any color morphing, and switch from Byte to Float.

This is the kind of stuff that makes glTF and my library so ugly and exceedingly complex.

AleksandarCebov commented 2 years ago

Maybe I will able to do this change as well, just will take me little bit time to figure out how to do this.

vpenades commented 2 years ago

I fixed it already, or at least I think I did; check your test for more details (it could be good to add a combined color+texture morph as a case of stress test)

Actually, I am curious about how to display the model correctly; is there any online viewer that actually supports this?

AleksandarCebov commented 2 years ago

Not at the moment (at least not that I am aware of). I reported to Three.js, hopefully will get it there soon: https://github.com/mrdoob/three.js/issues/23467

vpenades commented 2 years ago

The thing would be to create a glTF model that we can verify it is correct, and submit it to the khronos gltf sample models

I think they would need 3 examples: color morph, uv morph, and color + uv morph

AleksandarCebov commented 2 years ago

I will create the sample models (maybe also make them part of my 3rd party tests).