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

Texture support for raw Mesh3D logging #4894

Merged
merged 30 commits into from
Feb 2, 2024
Merged

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jan 23, 2024

What

Meshes can now be logged with optional texture coordinates and an optional albedo texture!

Vertex texture coordinates are straight forward, implemented as a new component, mostly to avoid semantic confusion (position2d on mesh?) and being able to call the coordinates uv.

The (albedo) texture itself got a bit messy: The original plan was to refer to a separately logged image and I believe that's still where we want to go. But this messes with some parts of the viewer so I made the decision to put a TensorData based field on the material. The main drawback of that is that now you can't inspect the texture separately and the format restrictions are quite tight (this is in part also due to the mesh renderer itself though, so that's deeper than just not using the Image archetype). I believe in the future the material should instead have an entity reference to specify where the texture comes from.
Another pretty bad sideffect from this is that if you have model with a bunch of meshes using the same texture several times, it will send this texture each time to the viewer since there's no way to "instance" this.

... but let's perfect not be the enemy of well-it-works-for-the-most-important-usecases-so-let's-ship-this!

Change: Texture is now a component, making a lot of things nicer

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

@Wumpf Wumpf added enhancement New feature or request 📺 re_viewer affects re_viewer itself 🍏 primitives Relating to Rerun primitives labels Jan 23, 2024
Copy link

github-actions bot commented Jan 23, 2024

Size changes

Name main 4894/merge Change
plots.rrd 1372.16 kiB 194.56 kiB -85.82%

@teh-cmc teh-cmc self-requested a review January 24, 2024 08:13
@teh-cmc
Copy link
Member

teh-cmc commented Jan 24, 2024

I cannot compile the rust example:

$ cargo r -q -p raw_mesh
error[E0432]: unresolved imports `rerun::external::ecolor`, `rerun::external::re_memory`
  --> examples/rust/raw_mesh/src/main.rs:16:16
   |
16 |     external::{ecolor, re_log, re_memory::AccountingAllocator},
   |                ^^^^^^          ^^^^^^^^^ could not find `re_memory` in `external`
   |                |
   |                no `ecolor` in `external`

error[E0433]: failed to resolve: could not find `clap` in `rerun`
   --> examples/rust/raw_mesh/src/main.rs:126:19
    |
126 |     rerun: rerun::clap::RerunArgs,
    |                   ^^^^ could not find `clap` in `rerun`

error[E0425]: cannot find function `setup_native_logging` in crate `re_log`
   --> examples/rust/raw_mesh/src/main.rs:182:13
    |
182 |     re_log::setup_native_logging();
    |             ^^^^^^^^^^^^^^^^^^^^ not found in `re_log`

Some errors have detailed explanations: E0425, E0432, E0433.
For more information about an error, try `rustc --explain E0425`.
error: could not compile `raw_mesh` (bin "raw_mesh") due to 3 previous errors

@teh-cmc
Copy link
Member

teh-cmc commented Jan 24, 2024

Python example works great but there are some issues in the data views.

In the outer data view, the material shows up as (empty):
image

In the data view of the material itself:

  • The albedo_factor shows up as (empty) which is... weird? 🤔 what does it mean for an albedo factor to be empty?
  • The texture shows its dimensions but there's no way to preview it. Any chance we can re-use the existing preview machinery for images/tensors?
    image

if albedo_factor.is_some() {
mesh = mesh.with_mesh_material(rerun::datatypes::Material {
albedo_factor: albedo_factor
.map(|[r, g, b, a]| ecolor::Rgba::from_rgba_unmultiplied(r, g, b, a).into()),
albedo_texture: None, // TODO(andreas): This would require loading the right texture file and converting it to `TensorData`.
Copy link
Member

Choose a reason for hiding this comment

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

Why not do it for the rust example too?

Copy link
Member Author

@Wumpf Wumpf Jan 24, 2024

Choose a reason for hiding this comment

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

I ran out of time and didn't see a quick & easy path towards that. Afaik the gltf loader doesn't load the image itself, so I need an extra path that loads the images from disk etc.

@teh-cmc
Copy link
Member

teh-cmc commented Jan 24, 2024

Would be nice to update the screenshots in raw_mesh readmes (python & rust).

They look so sad right now as they rely on just the albedo factor, it's so much better looking now.

@Wumpf
Copy link
Member Author

Wumpf commented Jan 24, 2024

Would be nice to update the screenshots in raw_mesh readmes (python & rust).

Unfortunately I didn't update the Rust example (and apparently also broke it :/) though to upload the texture. Guess I shouldn't leave this business unfinished

@Wumpf
Copy link
Member Author

Wumpf commented Jan 24, 2024

The albedo_factor shows up as (empty) which is... weird? 🤔 what does it mean for an albedo factor to be empty?

Albedo factor showed like that before when it's null. Don't know what else to write there. Isn't (empty) what we usually show for null

The texture shows its dimensions but there's no way to preview it. Any chance we can re-use the existing preview machinery for images/tensors?

Yeah this sucks, but I found this really hard to fix that right now with the setup of the texture not being a Image archetype. See code comments & pr description. Again, this was outside my time budget

Copy link
Member

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

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

Works great and I agree with everything you said 👍 🚢

crates/re_types/definitions/rerun/datatypes/material.fbs Outdated Show resolved Hide resolved
crates/re_renderer/src/lib.rs Outdated Show resolved Hide resolved
crates/re_data_ui/src/material.rs Outdated Show resolved Hide resolved
crates/re_space_view_spatial/src/visualizers/meshes.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf added the do-not-merge Do not merge this PR label Jan 25, 2024
@Wumpf
Copy link
Member Author

Wumpf commented Jan 29, 2024

Blocked by a crash in arrow when logging only a color and no image

@Wumpf
Copy link
Member Author

Wumpf commented Jan 30, 2024

@Wumpf Wumpf added the blocked can't make progress right now label Jan 30, 2024
@Wumpf Wumpf removed blocked can't make progress right now do-not-merge Do not merge this PR labels Feb 1, 2024
@Wumpf Wumpf requested a review from teh-cmc February 1, 2024 17:05
@teh-cmc
Copy link
Member

teh-cmc commented Feb 2, 2024

The fact that we cannot create a 2D view of the albedo texture when inspecting it because it is not in itself an entity is an interesting and extremely frustrating limitation of our current system.

I'll probably open an issue actually...

image

crates/re_space_view_spatial/src/mesh_cache.rs Outdated Show resolved Hide resolved
examples/rust/raw_mesh/src/main.rs Show resolved Hide resolved
@teh-cmc teh-cmc merged commit b312a4f into main Feb 2, 2024
43 of 44 checks passed
@teh-cmc teh-cmc deleted the andreas/log-textured-meshes branch February 2, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request include in changelog 🍏 primitives Relating to Rerun primitives 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants