Fixed non draco export #17
Conversation
|
Any updates on this? @Doerge |
|
Yes, I'll carve out some time to test it later today, so we can include it before beginning work on the Loader! |
|
I've got a fix for #12 built on top of this, I can push the commit to this pull as well if you want? |
|
Sorry, just saw your comment now. Let's keep it separate, but I'll keep the ball rolling in the evenings to get stuff merged. Please review my additions in the other PR #20 |
|
Hi, I think you should push the commit to this PR so it's all bundled with the rest of this conversation. I've allowed edits by maintainers so you should be able to do it. I was silly forgetting the Z -> Y conversion. I logged #21 since the Z up -> Y up conversion needs sorted a bit better, but for now it's not getting in the way. It doesn't look like you used the texCoords min and max down in the accessor. I might be wrong, but I don't think we need to calculate those or the normal min and maxes either? I'll need to double check. |
There was a problem hiding this comment.
I've allowed edits by maintainers so you should be able to do it.
I didn't know that was possible! 🤯
Great that you created #21 ! Agreed it should be an option.
Ah. I thought validators might fail us, if we didn't keep proper track of min/max. I plugged them in for now. They don't hurt other than taking a bit of extra processing.
Texcoords might also have use cases outside [0, 1] if the user is super creative with the UV layout and wrapping modes. Not sure if Normals are guaranteed to be normalized by the gltf spec, but that would be a reasonable assumption 😄
| @@ -145,6 +146,9 @@ public void SerializeToGLB(in MemoryStream memoryStream) | |||
| bv.byteOffset = bv.bufferRef.binaryOffset; | |||
| } | |||
|
|
|||
| //Have to get rid of the other buffers so only the new buffer | |||
| //all the data was dumped into remains | |||
| buffers.Clear(); | |||
There was a problem hiding this comment.
Man... how did that even work before? 👍 Thanks!
While reviewing this I realize I may have misunderstood the spec initially: I now think it is valid to create multiple binary buffers, instead of mashing everything into one and using views to discern the data blobs. Not sure what the implications are of that yet. The validator fails with:
{
"code": "ACCESSOR_TOTAL_OFFSET_ALIGNMENT",
"message": "Accessor's total byteOffset 397965 isn't a multiple of componentType length 4.",
"severity": 0,
"pointer": "/accessors/0"
},
Maybe that's a side effect of this weird choice. Will create a separate ticket for that though.
There was a problem hiding this comment.
Yes, I think this changed. While we probably can cram multiple things into a single buffer it's unnecessarily complicated and makes the code and final output file confusing. The validator error makes sense to me since we're probably mixing floats/bytes/integers all in one buffer and just fiddling with offsets. It's not super imperative though so we'll get to #22 when either of us has a chance.
Resolves #2
It might be worth taking a close look at this one to see if I did anything crazy.
I had the save file dialog created based on whether its the binary (.glb) or text (.gtlf) and added back the other Rhino.Gets for the export options.
I also think you forgot to add the json ignore attribute over the buffers raw bytes field.
In the SerializeToGLB I made it so the buffers are cleared before you add the packed buffer. Otherwise all the BufferViews look at whatever the first buffer was and not the packed buffer since it's at the end.