UNSIGNED_INT Fix for Vertex Indices #137
Conversation
…s when allowed and needed 1) Switched from global use of short unsigned to new index_t (and where necessary, signed_index_t) in COLLADA2GLTFWriter. 2) Put a few functions into the COLLADA2GLTF::Writer class that weren't there so they could see the new types. 3) Made COLLADA2GLTF::Writer use UNSIGNED_INT buffers where allowed and required. This will NOT work as-is if UNSIGNED_SHORT buffers are specified instead due to vector data size.
|
Thanks @KermMartian! @lasalvavida do you happen to have a minute to review? Thanks! |
|
Thank you for the contribution @KermMartian, I will try to review sometime this week |
There was a problem hiding this comment.
Done for now.
Probably the big takeaway is this:
I think the preferred implementation would be to drop the command line option, and precompute the required number of indices during mesh generation, then choose the appropriate type for the container (probably excluding UNSIGNED_BYTE)
Eventually we will probably want to add support for forcing uint16 indices with mesh splitting, but for now I think the user experience will be better if we automatically use the appropriate type, instead of setting it globally, and throw a warning if a mesh exceeds uint32.
| if(NOT OpenCOLLADA) | ||
| add_subdirectory(dependencies/OpenCOLLADA/modules/COLLADASaxFrameworkLoader) | ||
| set(OpenCOLLADA COLLADASaxFrameworkLoader) | ||
| if(NOT DEFINED COLLADA_LIBRARIES AND NOT DEFINED COLLADA_FRAMEWORK_LIBRARIES) |
There was a problem hiding this comment.
While I'm not opposed to build changes allowing specifying alternate locations for libraries as in #125, please remove this and open a separate pull request for it.
|
|
||
| bool writeNodeToGroup(std::vector<GLTF::Node*>* group, const COLLADAFW::Node* node); | ||
| bool writeNodesToGroup(std::vector<GLTF::Node*>* group, const COLLADAFW::NodePointerArray& nodes); | ||
| GLTF::Texture* fromColladaTexture(const COLLADAFW::EffectCommon* effectCommon, COLLADAFW::SamplerID samplerId); | ||
| GLTF::Texture* fromColladaTexture(const COLLADAFW::EffectCommon* effectCommon, COLLADAFW::Texture texture); | ||
|
|
||
| void interpolateTranslation(float* base, std::vector<float> input, std::vector<float> output, signed_index_t index, |
There was a problem hiding this comment.
What was the motivation behind making these into member functions? I typically leave functions out of the class declaration unless they:
- need access to class members
- are published as an interface to be used elsewhere
| @@ -35,14 +40,21 @@ namespace COLLADA2GLTF { | |||
| std::map<COLLADAFW::UniqueId, GLTF::Mesh*> _skinnedMeshes; | |||
| std::map<COLLADAFW::UniqueId, GLTF::Image*> _images; | |||
| std::map<COLLADAFW::UniqueId, std::tuple<std::vector<float>, std::vector<float>>> _animationData; | |||
| bool _use_uint_indices = false; | |||
There was a problem hiding this comment.
It doesn't look like this is ever assigned to
| public: | ||
| Writer(GLTF::Asset* asset, COLLADA2GLTF::Options* options, COLLADA2GLTF::ExtrasHandler* handler); | ||
| void setUseUintIndices(const bool use_uint_indices); |
There was a problem hiding this comment.
This is also never used. Is it just leftover from before the property was added to GLTF::Options?
| @@ -101,6 +101,10 @@ int main(int argc, const char **argv) { | |||
| parser->define("qj", &options->jointQuantizationBits) | |||
| ->description("joint indices and weights quantization bits used in Draco compression extension"); | |||
|
|
|||
| parser->define("uint", &options->useUintIndices) | |||
| ->defaults(false) | |||
| ->description("use OES_element_index_uint extension to allow 32-bit vertex indices"); | |||
There was a problem hiding this comment.
Maybe change this description to something like "Write indices as unsigned 32-bit integers" and change the option to useUint32Indices for clarity. The idea of using OES_element_index_uint is abstracted out to the person writing the glTF loader and may be confusing for people who are just looking to convert models.
edit: I would prefer following the top-level comment and removing this
| @@ -306,41 +306,39 @@ bool COLLADA2GLTF::Writer::writeNodeToGroup(std::vector<GLTF::Node*>* group, con | |||
| // Instance Geometries | |||
| const COLLADAFW::InstanceGeometryPointerArray& instanceGeometries = colladaNode->getInstanceGeometries(); | |||
| size_t count = instanceGeometries.getCount(); | |||
| if (count > 0) { | |||
|
|
||
| namespace COLLADA2GLTF { | ||
| class Writer : public COLLADAFW::IWriter { | ||
| private: | ||
| typedef uint32_t index_t; | ||
| typedef int32_t signed_index_t; |
There was a problem hiding this comment.
Why does this exist? Indices are never signed.
In general, I would prefer not to macro out types like this since I think it lowers the readability of the code, especially when there are only three possible values anyway via the spec.
5121 (UNSIGNED_BYTE), 5123 (UNSIGNED_SHORT) or 5125 (UNSIGNED_INT)
I think the preferred implementation would be to drop the command line option, and precompute the required number of indices during mesh generation, then choose the appropriate type for the container (probably excluding UNSIGNED_BYTE)
| @@ -391,7 +389,7 @@ bool COLLADA2GLTF::Writer::writeLibraryNodes(const COLLADAFW::LibraryNodes* libr | |||
|
|
|||
| void mapAttributeIndices(const unsigned int* rootIndices, const unsigned* indices, int count, std::string semantic, std::map<std::string, GLTF::Accessor*>* attributes, std::map<std::string, std::map<int, int>>* indicesMapping) { | |||
| indicesMapping->emplace(semantic, std::map<int, int>()); | |||
| for (int i = 0; i < count; i++) { | |||
| for (size_t i = 0; i < count; i++) { | |||
There was a problem hiding this comment.
Thanks for these size_t fixes.
|
Apologies in advance if it's bad form to post on a closed PR; it looks like this is currently unmerged? Would it be correct to rebase this onto |
|
Sorry @KermMartian, didn't close this intentionally. Github just did it automatically when I deleted the 2.0 branch. I retargeted your PR for master and reopened it. |
|
@KermMartian, I'm closing this in favor of #159. Feel free to open up a separate pull request with any of your smaller changes from this PR as mentioned in my review. |
To resolve #123, attempt to use UNSIGNED_INT values for vertex indices iff the --uint command-line flag is specified and more than 2^16 unique vertex indices have been used in this primitive. Changes are also meant to make twiddling with this easier in the future by introducing index_t and signed_index_t types in
COLLADA2GLTF::Writer.