Twinklebear / tobj

Tiny OBJ Loader in Rust
MIT License
235 stars 47 forks source link

Vertex colors #44

Closed luringens closed 3 years ago

luringens commented 3 years ago

Hi @Twinklebear, thanks so much for making and maintaining tobj! I wanted to make use of a model with vertex colors and decided to try to add the functionality to my favourite obj parsing crate, resolving #26 🙂

The changes are fairly straightforward:

I still need to make this work with the merge_identical_points feature. As the vertex colors currently share indices with vertices, this is not entirely trivial. I did add a test for this, which is currently failing. As this test can only run on nightly with the merging feature, this will not show up with the current CI configuration.

Twinklebear commented 3 years ago

Thanks @stisol, this looks great! The interaction with merge_identical_points is an interesting one, since the vertex + color are both part of the "position". So if two vertices have the same position but different colors, they can't really be merged. One possible option would be to introduce a new color_indices vec on the mesh, which could then let each vertex have a different color index than its position index. We could maybe also use that to merge repeated vertex colors, since it looks like the way they're specified it's not possible to reuse them (e.g., all vertex colors are blue or something)

luringens commented 3 years ago

Thanks for the feedback! Yeah, inline vertex colors are by their nature very memory inefficient. I think you're right - introducing a color index and merging vertex colors like the other attributes feels like the most natural way to support them in this context.

I added a vertex_color_indices field to Mesh which is only included when merging is enabled, as that is the only context where it can be non-empty. If vertex colors are present when merging, it is constructed by cloning the vertex position indices array before feeding it to the usual merge_identical_points function.

This works as expected in my simple reordered quad unit test. I also temporarily (and messily) converted the program I'm working on make use of reordering to give the feature a more thorough test run, and it worked as expected. 20k vertices, with only 3 elements in the vertex_color array, and the vertex color index colored everything correctly 🙂

Since the merging unit test only runs on nightly, do you want me to add a commit to this PR that adds a CI job to compile and test on nightly will all features enabled? This snippet appended to the end of rust.yml the seems to work:

    nightly_features_linux:
      runs-on: ubuntu-latest
      steps:
      - uses: actions/checkout@v2
      - uses: actions-rs/toolchain@v1
        with:
          profile: minimal
          toolchain: nightly
          override: true
      - name: Build
        run: cargo build --verbose
      - name: Run tests
        run: cargo test --verbose --all-features 
Twinklebear commented 3 years ago

Great! Yeah adding a nightly CI runner would be good, I usually am not developing on nightly so it'll be useful to make sure things don't break

Twinklebear commented 3 years ago

Awesome, thanks @stisol ! I'll cut a release shortly with this addition