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

Allow color specification on Asset3D #7458

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

EtaLoop
Copy link

@EtaLoop EtaLoop commented Sep 20, 2024

What

The idea is to reproduce behavior of Mesh3D regarding color modification on Asset3D :

  • add albedo_factor
  • add vertex_colors as in Mesh3D

In order to overwrite white default texture of Asset3D loaded from stl and obj (gltf already have albedo_factor in file).

See: #5253

Screenshot from 2024-09-18 16-05-54

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

Keyvan Goddard added 3 commits September 30, 2024 14:43
- add attribute `albedo_factor` and method `with_albedo_factor` to archetype `Asset3D`.
- update stl/obj loader : `load_stl_from_buffer` and `load_obj_from_buffer` to process `albedo_factor`.
  Remove `albedo_texture_buffer`, `albedo_texture_format` from `Asset3D`
- add attribute `vertex_colors` and method `with_vertex_colors` to archetype `Asset3D`.
- update stl/obj loader : `load_stl_from_buffer` and `load_obj_from_buffer` to process `vertex_colors`.
@EtaLoop EtaLoop marked this pull request as ready for review September 30, 2024 13:03
@Wumpf Wumpf self-requested a review October 1, 2024 09:29
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!! 🙂

AlbedoFactor makes a lot of sense to me since this can be applied on all mesh/model formats, would love to get this in. But I don't see yet how we can add vertex_colors in a tractable & defined way. However, maybe we can come up with some rule for it?
Very relatedly, we have to keep in mind that more model formats may be added in the future and we need to avoid ugly surprises in that context. Therefore, I think all additional properties on Assed3D should be "postprocesses" on the loaded mesh. This makes it more robust against future formats and makes it a lot easier to specify what they do.

crates/viewer/re_renderer/src/importer/obj.rs Outdated Show resolved Hide resolved
crates/viewer/re_renderer/Cargo.toml Outdated Show resolved Hide resolved
crates/viewer/re_renderer/src/importer/stl.rs Outdated Show resolved Hide resolved
@Wumpf
Copy link
Member

Wumpf commented Oct 3, 2024

let me know when you think this is ready for review again! :)
(or use the little "re-request review" button so this shows up in my review queue again)

@EtaLoop EtaLoop force-pushed the color-in-asset3d branch 2 times, most recently from 36c02cd to 87751a4 Compare October 3, 2024 13:52
@EtaLoop EtaLoop requested a review from Wumpf October 11, 2024 08:17
EtaLoop added 2 commits October 11, 2024 11:19
- Add #derive[(Clone)] instead of impl Clone for MeshInstance
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