Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vertex colors #44

Merged
merged 4 commits into from
Jul 30, 2021
Merged

Vertex colors #44

merged 4 commits into from
Jul 30, 2021

Conversation

luringens
Copy link
Contributor

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 added a new field called vertex_color to Mesh, following the style of normals and texcoords, and updated the README accordingly.
  • After successfully parsing a position from a vertex line v, it will now add the vertex color on the same line if it is present.
  • I added a FaceColorOutOfBounds error variant to match the existing OutOfBounds errors for normals and uv coords.
  • I made parse_floatn take the SplitWhitespace by a mutable reference instead of by ownership in order to allow the iterator to be used again. As it will now only consume n items, it is now called a second time on v lines to see if there is a second trio of floats on it.
  • All the tests still pass, and I added a simple_triangle_colored test that checks that the triangle file with vertex colors added is still parsed the same way, with the extra vertex color array now filled.

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.

Stian Soltvedt added 2 commits July 20, 2021 15:08
@Twinklebear
Copy link
Owner

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
Copy link
Contributor Author

luringens commented Jul 22, 2021

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 

@luringens luringens marked this pull request as ready for review July 22, 2021 10:54
@Twinklebear
Copy link
Owner

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
Copy link
Owner

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

@Twinklebear Twinklebear merged commit 02f4e0a into Twinklebear:master Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants