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

Add optional solid/filled (triangle mesh) rendering to Boxes3D and Ellipsoids. #6953

Merged
merged 22 commits into from
Jul 24, 2024

Conversation

kpreid
Copy link
Collaborator

@kpreid kpreid commented Jul 21, 2024

What

With these changes, Boxes3D and Ellipsoids can now be viewed as solid objects. This is part of the work on #1361 and the mechanisms added here will generalize to other shapes.

  • Add new component type SolidColor. This is identical to Color except that, on shapes where it applies, it sets the color of surfaces instead of lines. Whether its color is transparent controls whether the surfaces are drawn at all.
  • Add SolidColor to the Boxes3D and Ellipsoids archetypes.
  • Add support for solid colors to those archetypes’ visualizers.
  • Add proc_mesh::SolidCache to cache the calculation of triangle meshes.
  • Add proc_mesh::ProcMeshKey::Cube to allow the cached mech mechanism to generate solid cubes.

Screenshot 2024-07-20 at 17 36 01
Screenshot 2024-07-20 at 17 35 12

Future work (things this PR doesn't do):

  • Viewer UI needs a version of the color picker that lets you pick "fully transparent".
  • The line renderer does not play well with adjacent faces, causing line width to be halved some of the time. This could be fixed with something like Render points with per-fragment depth. #6508, simple depth offsets, or something else.
  • Getting naming and semantics right:
    • Should the colors of Boxes3D be renamed line_colors, or something like that? I think so, but that would be a breaking change, so I didn't in this PR.
    • Should Color be renamed?
    • Should there be all 3 of SolidColor, LineColor, and Color` with some override policy between them?
    • Should SolidColor be called SurfaceColor instead?
    • How should SolidColor interact with annotation contexts?
  • Figure out whether instanced meshes are the right choice for performance.

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
    • The Ellipsoids archetype has a renamed field but that isn't released yet, so doesn't need noting.

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

@kpreid kpreid added 📺 re_viewer affects re_viewer itself 🍏 primitives Relating to Rerun primitives include in changelog labels Jul 21, 2024
@kpreid kpreid marked this pull request as draft July 21, 2024 01:21
@kpreid kpreid marked this pull request as ready for review July 22, 2024 04:36
@Wumpf Wumpf self-requested a review July 22, 2024 08:27
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.

looking great, almost good to go!

lots of cans of worms here (annotation context interaction, lack of tagged components, backwards compat questions of existing Color, lack of generalized transparency, ...) but I think they're all sidestepped well enough here that we can move forward 😄

We definitely need to do something about the color view/edit ui, but this can go separately

docs/snippets/all/archetypes/box3d_batch.py Outdated Show resolved Hide resolved
crates/viewer/re_edit_ui/src/lib.rs Outdated Show resolved Hide resolved
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.

(that was meant to be a request changes :))

@jleibs
Copy link
Member

jleibs commented Jul 22, 2024

Spent some time talking through this with @Wumpf and @nikolausWest this morning.

Unfortunately the consensus is that introducing support for separable line vs solid colors leads down a complexity pathway we would like to avoid until we have:

  1. Implemented tagged components
  2. Refactored how we handle annotation context.

I think the best compromise solution on the complexity scale is to, instead, introduce a new enum value:

enum FillMode {
  Wireframe,
  Solid
}

Regardless of choice, the single Color component would still get used. This gives us a nice single value toggleable enum in the UI to switch the modes without needing to worry about disabling/enabling the two separately.

@kpreid
Copy link
Collaborator Author

kpreid commented Jul 22, 2024

Regardless of choice, the single Color component would still get used.

I see a problem with this schema: it prevents drawing solid and lines simultaneously with different colors (e.g. one might use the solid color for "object type" and the lines for "state"). I suppose “use two entities or instances" is a reasonable interim way to achieve that, though. I'll update this PR as you propose.

@jleibs
Copy link
Member

jleibs commented Jul 22, 2024

it prevents drawing solid and lines simultaneously with different colors

Yeah, definitely acknowledged as a shortcoming. But I think it's one we can improve in the future with #6889

@nikolausWest
Copy link
Member

Apologies on the back and forth on this one @kpreid. I realize we sent you in the other direction initially

This merge commit was made with `git merge --strategy=ours`
and doesn't actually merge any changes, just, essentially,
mark the second parent (the old line of development) as
part of the history.
@kpreid
Copy link
Collaborator Author

kpreid commented Jul 24, 2024

I've now pushed the requested changes. There is now no SolidColor component, and instead a FillMode component. This is, overall, a lot simpler. I also discovered, and fixed, issue #6973, because fixing it was easier and keeps the visualizer code parallel.

(Perhaps someday — or now — it would make the most sense to have Boxes3DVisualizer, EllipsoidsVisualizer, and other shape archetype visualizers build on a shared ProcMeshVisualizer.)

You may wish to note that the history of this PR branch now includes a dummy merge commit. This is because I found it easiest to re-develop the new design off of main rather than undoing all the SolidColor changes, but for the sake of preserving GitHub's code review comment linkages I wanted to keep the old commits present.

@kpreid
Copy link
Collaborator Author

kpreid commented Jul 24, 2024

New screenshots (without the line+surface rendering that is no longer supported):

image image

@Wumpf Wumpf self-requested a review July 24, 2024 07:32
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.

Looks solid! (pun intended)

I added an editor to the new enum, our ui fallback for those is unfortunately not working, so that one showed null (we should really start looking into generating edit uis automatically for trivial types - Opened an issue for this here now #6974)
image

Now with the editor it's possible to switch the mode in the ui 🎉

Screen.Recording.2024-07-24.at.10.12.11.mov

Comment on lines +107 to +108
// We must perform this transform to fully account for the per-instance
// transform, which is separate from the entity's transform.
Copy link
Member

Choose a reason for hiding this comment

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

thanks for clarifying this for the next person (me) who gets confused by this 😄

@Wumpf
Copy link
Member

Wumpf commented Jul 24, 2024

pried a bit into contributor ci issues, but will have to take this separately

@Wumpf Wumpf merged commit 44dc619 into rerun-io:main Jul 24, 2024
30 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants