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

Different icon for empty entity paths #5338

Merged
merged 7 commits into from
Mar 4, 2024
Merged

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Feb 28, 2024

What

This PR adds a subtly different icon for "empty" entities. By "empty", I mean "no component logged on the current timeline". This means that changing timeline may potentially change the icon of some entity (though it takes a somewhat contrived example to do so).

TODO:

image image image image

Fixes #5302

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!

@abey79 abey79 added ui concerns graphical user interface include in changelog labels Feb 28, 2024
@nikolausWest
Copy link
Member

These look a little strange, otherwise great!
Screenshot 2024-02-28 at 17 14 45

@abey79
Copy link
Member Author

abey79 commented Feb 28, 2024

@nikolausWest good spot! These are an oversight from my previous PR. Will fix asap.

edit: #5341

@abey79 abey79 marked this pull request as ready for review February 29, 2024 19:31
@teh-cmc teh-cmc self-requested a review March 1, 2024 07:54
Comment on lines 577 to 581
.with_icon(if tree.entity.components.is_empty() {
&re_ui::icons::ENTITY_EMPTY
} else {
&re_ui::icons::ENTITY
})
Copy link
Member

Choose a reason for hiding this comment

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

There is an inconsistency here: the data UI uses DataStore::all_components which is timeful, while this uses the EntityTree which is timeless.

I.e. scrubbing the time cursor might change the icon that appears in the blueprint panel and elsewhere, while that won't happen in the time panel.

I think that's fine (unless we want both to be timeless), but deserves a comment to specify that this was intentional at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed, both were actually timeless, but instance_path_icon() accepting a Query argument was very confusing. I changed it to accept a Timeline instead, which is all it needs.

Also, I made both the streams and the blueprint have their icons based on the current timeline (in my original commit, the streams icon was based on all timeline).

@emilk
Copy link
Member

emilk commented Mar 1, 2024

nice!

@abey79 abey79 merged commit eaec30f into main Mar 4, 2024
34 checks passed
@abey79 abey79 deleted the antoine/empty-entities-icon branch March 4, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the same icons for entities in blueprint and streams trees
4 participants