Spreadsheet: support navigating instance trees #125293

Merged
Jacques Lucke merged 54 commits from JacquesLucke/blender:spreadsheet-separate-instances into main 2024-07-29 20:42:25 +02:00
Member

Previously, it was not possible to see detailed information about instances in the spreadsheet. Only the attributes on the top level instances were shown. Now, all nested instances can be inspected too.

Combined with #114910 this will make inspecting more complex geometry with the spreadsheet much more feasible. It's also an important part of integrating grease pencil into geometry nodes because it makes it more obvious how layers are converted to curve instances.

The data-selection is split into two separate tree views now. One that selects the geometry from the instance tree, and one that's used to select the geometry component and domain within that geometry. We found that this works better than combining both tree views into one (we tried that in #124186).

image

Previously, it was not possible to see detailed information about instances in the spreadsheet. Only the attributes on the top level instances were shown. Now, all nested instances can be inspected too. Combined with #114910 this will make inspecting more complex geometry with the spreadsheet much more feasible. It's also an important part of integrating grease pencil into geometry nodes because it makes it more obvious how layers are converted to curve instances. The data-selection is split into two separate tree views now. One that selects the geometry from the instance tree, and one that's used to select the geometry component and domain within that geometry. We found that this works better than combining both tree views into one (we tried that in #124186). ![image](/attachments/0c73ef3c-0286-44db-a7bb-6a7d0094be89)
Jacques Lucke added 38 commits 2024-07-23 11:25:59 +02:00
Fix handling of selection filtering
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
f09f6fd53a
don't show as activatable for some items
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
08457ec710
Jacques Lucke changed title from Spreadsheet: support navigating instance trees (Alternative 2) to Spreadsheet: support navigating instance trees (alternative 2) 2024-07-23 11:26:17 +02:00
Jacques Lucke requested review from Simon Thommes 2024-07-23 11:30:50 +02:00
Jacques Lucke requested review from Hans Goudey 2024-07-23 11:30:52 +02:00
Member

I really like the direction of this PR, especially compared to the alternative. IMO it makes the structure a lot more clear and the individual sets easier to inspect.

One thought is also that this scales quite nicely with the idea of having multiple geometry sets within the original data of an object in the future.

Some initial feedback without full testing:

  • It would be good imo to expand the elements of the top tree by default to give a full overview of the hierarchy
  • I think eventually we will need individual scrolling for the two trees in some way, since the first one can become quite long and should not make it inconvenient to choose the spreadsheet context
  • It would be great to show the user count in the top tree next to the name of the geometry set. This would, beyond just being useful information, take away some of the confusion that comes with everything just being named Geometry by default, by identifying elements by their characteristic user count.
    image
  • there is still the question of the icon for geometry sets. I think in this design it is a bit less bad, since they are separate trees, but showing the instance icon next to what represents the actual data that is being instanced seems a bit odd to me. Same for use of the the Geometry Nodes icon
    -- I'm actually not sure why exatly they have different icons here. Is that just because one is a top level set?
  • maybe there should be a title for each of the trees to communicate what they represent now that there are two. It seems un-ideal to assume people know what they mean
  • when there is only one geometry set (e.g. default cube) it does look a bit goofy to have a tree of just one element that is highlighted as active. That's not terrible, but since this is going to be the case quite often, it would be nice to find a better UI solution for this case.
    Clipboard - July 23, 2024 11_42 AM.png
I really like the direction of this PR, especially compared to the alternative. IMO it makes the structure a lot more clear and the individual sets easier to inspect. One thought is also that this scales quite nicely with the idea of having multiple geometry sets within the original data of an object in the future. Some initial feedback without full testing: - It would be good imo to expand the elements of the top tree by default to give a full overview of the hierarchy - I think eventually we will need individual scrolling for the two trees in some way, since the first one can become quite long and should not make it inconvenient to choose the spreadsheet context - It would be great to show the user count in the top tree next to the name of the geometry set. This would, beyond just being useful information, take away some of the confusion that comes with everything just being named `Geometry` by default, by identifying elements by their characteristic user count. ![image](/attachments/b7e2597b-efcd-4b6a-ba8b-59c4785c338a) - there is still the question of the icon for geometry sets. I think in this design it is a bit less bad, since they are separate trees, but showing the instance icon next to what represents the actual data that is being instanced seems a bit odd to me. Same for use of the the Geometry Nodes icon -- I'm actually not sure why exatly they have different icons here. Is that just because one is a top level set? - maybe there should be a title for each of the trees to communicate what they represent now that there are two. It seems un-ideal to assume people know what they mean - when there is only one geometry set (e.g. default cube) it does look a bit goofy to have a tree of just one element that is highlighted as active. That's not terrible, but since this is going to be the case quite often, it would be nice to find a better UI solution for this case. ![Clipboard - July 23, 2024 11_42 AM.png](/attachments/b7bb66cc-8c59-4fb6-a0c6-52879a9f63e1)
Jacques Lucke added 3 commits 2024-07-23 12:21:23 +02:00
Author
Member

I'm actually not sure why exatly they have different icons here. Is that just because one is a top level set?

Yes, that's what it does currently, but there is no specific reason for that, I'm just trying different variations to see if something works.

> I'm actually not sure why exatly they have different icons here. Is that just because one is a top level set? Yes, that's what it does currently, but there is no specific reason for that, I'm just trying different variations to see if something works.
Jacques Lucke added 2 commits 2024-07-23 12:47:15 +02:00
Jacques Lucke added 1 commit 2024-07-23 12:49:37 +02:00
Jacques Lucke added 1 commit 2024-07-23 13:05:56 +02:00
Jacques Lucke added 2 commits 2024-07-24 14:41:47 +02:00
Member

I did a mockup for a potential geometry (set) icon:
image

I did a mockup for a potential geometry (set) icon: ![image](/attachments/e8f54901-e7b6-4e37-9e87-a4b15ece376c)
Author
Member

Nice, thanks! Looks good enough to me right now, but I don't know too much about icons. Will mention it in the user interface module chat.

Nice, thanks! Looks good enough to me right now, but I don't know too much about icons. Will mention it in the user interface module chat.
Jacques Lucke added 2 commits 2024-07-26 13:04:32 +02:00
Author
Member

I added your icon to the patch. Maybe that's good enough for now and we can potentially do another pass on it separately?

I added your icon to the patch. Maybe that's good enough for now and we can potentially do another pass on it separately?
Member

I'd like to properly sign something off before we merge. This mockup was also done pretty quickly and isn't very clean. But that should be easy to redo once a design is settled on.
We should be able to do that next week.

I'd like to properly sign something off before we merge. This mockup was also done pretty quickly and isn't very clean. But that should be easy to redo once a design is settled on. We should be able to do that next week.
Jacques Lucke changed title from Spreadsheet: support navigating instance trees (alternative 2) to Spreadsheet: support navigating instance trees 2024-07-27 23:08:51 +02:00
Member

Iterated a bit more over the icon, so here is a version with overlap, proper rounding and aligned to a grid that should bring it more in line with the rest of the icon designs.

Personally I'd go with either the filled version with overlap or the non-filled without overlap. Will ask in the UI module again.

Filled
image

Lines
image

Iterated a bit more over the icon, so here is a version with overlap, proper rounding and aligned to a grid that should bring it more in line with the rest of the icon designs. Personally I'd go with either the filled version with overlap or the non-filled without overlap. Will ask in the UI module again. Filled ![image](/attachments/ddf289bf-eaa3-48e6-a407-d3f15b340fe6) Lines ![image](/attachments/f2deef31-6dc2-44b1-809a-75555e844a99)
First-time contributor

By arranging it a little differently, I think the shapes are better understood.
This way, three corners of the square are visible and the lower edge of the triangle is not aligned with the upper edge of the square.

By arranging it a little differently, I think the shapes are better understood. This way, three corners of the square are visible and the lower edge of the triangle is not aligned with the upper edge of the square.
Member

By arranging it a little differently, I think the shapes are better understood.

I tried this, but it doesn't work imo. The balance is quite off.
The shape readability works well enough imo. I'ts not about the exact shapes and order anyways, just the concept.

Since this iteration got a bunch of positive feedback in the UI module let's go with this for now.

geometry_set.svg

> By arranging it a little differently, I think the shapes are better understood. I tried this, but it doesn't work imo. The balance is quite off. The shape readability works well enough imo. I'ts not about the exact shapes and order anyways, just the concept. Since this iteration got a bunch of positive feedback in the UI module let's go with this for now. ![geometry_set.svg](/attachments/def24c4d-71eb-4d23-95b6-7098c09d1224)
Jacques Lucke added 2 commits 2024-07-29 15:29:01 +02:00
Author
Member

I updated the patch to include the new icon. Thanks for that!

I updated the patch to include the new icon. Thanks for that!
Simon Thommes approved these changes 2024-07-29 17:14:55 +02:00
Simon Thommes left a comment
Member

IIRC the icon was the last piece this PR was waiting on from what we talked about.

Some additional thoughts:

  • I do still think it is a bit odd how the panels can be collapsed individually and the dependency between them is not really clear immediately. So it would be nice to see if it was possible to make these kinds of panels un-collapsable in a separate patch.
  • Similarly, I think, in the future, the ability to scroll these trees individually would be very useful. But that is besides this initial PR.
  • Since the number next to the Geometry Set name is indeed not entirely self-explanatory, it would be great to have a tooltip for it, if possible
  • To create another connection between the geometry sets in the top tree and the instances in the spreadsheet, I think it would make sense to display the user-count there as well (in the same way). This would also help identifying an exact geometry when duplicate names are involved.

One last thought/suggestion:
Since the title Domain wasn't a perfect fit, how about going the more generic route and calling it Data. Since this tree refers to the part of the geometry data to be inspected, that would generally make sense imo.

Both works imo, so since the other things I mentioned can all be done separately anyways, I'll already approve the PR at this point.

IIRC the icon was the last piece this PR was waiting on from what we talked about. Some additional thoughts: - I do still think it is a bit odd how the panels can be collapsed individually and the dependency between them is not really clear immediately. So it would be nice to see if it was possible to make these kinds of panels un-collapsable in a separate patch. - Similarly, I think, in the future, the ability to scroll these trees individually would be very useful. But that is besides this initial PR. - Since the number next to the Geometry Set name is indeed not entirely self-explanatory, it would be great to have a tooltip for it, if possible - To create another connection between the geometry sets in the top tree and the instances in the spreadsheet, I think it would make sense to display the user-count there as well (in the same way). This would also help identifying an exact geometry when duplicate names are involved. One last thought/suggestion: Since the title `Domain` wasn't a perfect fit, how about going the more generic route and calling it `Data`. Since this tree refers to the part of the geometry data to be inspected, that would generally make sense imo. Both works imo, so since the other things I mentioned can all be done separately anyways, I'll already approve the PR at this point.
Author
Member

Since the number next to the Geometry Set name is indeed not entirely self-explanatory, it would be great to have a tooltip for it, if possible

Would be nice indeed, but at a first glance this seems to be more tricky than one would hope.. That's because this number is not a standalone label uiBut but part of the uiBut of the entire item (implementation details). Good idea though, something to keep in mind.

To create another connection between the geometry sets in the top tree and the instances in the spreadsheet, I think it would make sense to display the user-count there as well (in the same way). This would also help identifying an exact geometry when duplicate names are involved.

Where would you display that exactly. Seems weird to have it next to Mesh.

Since the title Domain wasn't a perfect fit, how about going the more generic route and calling it Data. Since this tree refers to the part of the geometry data to be inspected, that would generally make sense imo.

Currently, I prefer Domain over Data...

> Since the number next to the Geometry Set name is indeed not entirely self-explanatory, it would be great to have a tooltip for it, if possible Would be nice indeed, but at a first glance this seems to be more tricky than one would hope.. That's because this number is not a standalone label `uiBut` but part of the `uiBut` of the entire item (implementation details). Good idea though, something to keep in mind. > To create another connection between the geometry sets in the top tree and the instances in the spreadsheet, I think it would make sense to display the user-count there as well (in the same way). This would also help identifying an exact geometry when duplicate names are involved. Where would you display that exactly. Seems weird to have it next to `Mesh`. > Since the title Domain wasn't a perfect fit, how about going the more generic route and calling it Data. Since this tree refers to the part of the geometry data to be inspected, that would generally make sense imo. Currently, I prefer `Domain` over `Data`...
Hans Goudey approved these changes 2024-07-29 20:26:30 +02:00
Hans Goudey left a comment
Member

Probably worth committing the new icon separately. But I think this is ready to go in. Using "Data" as a panel label is fine with me, though not sure it's really better TBH.

Probably worth committing the new icon separately. But I think this is ready to go in. Using "Data" as a panel label is fine with me, though not sure it's really better TBH.
@ -130,2 +130,3 @@
attributes_(other.attributes_),
almost_unique_ids_cache_(std::move(other.almost_unique_ids_cache_))
almost_unique_ids_cache_(std::move(other.almost_unique_ids_cache_)),
reference_user_counts_(std::move(other.reference_user_counts_))
Member

These are out of order

/home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh: In constructor ‘blender::bke::Instances::Instances(blender::bke::Instances&&)’:
/home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh:127:35: warning: ‘blender::bke::Instances::almost_unique_ids_cache_’ will be initialized after [-Wreorder]
  127 |   mutable SharedCache<Array<int>> almost_unique_ids_cache_;
      |                                   ^~~~~~~~~~~~~~~~~~~~~~~~
/home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh:121:35: warning:   ‘blender::SharedCache<blender::Array<int> > blender::bke::Instances::reference_user_counts_’ [-Wreorder]
  121 |   mutable SharedCache<Array<int>> reference_user_counts_;
      |                                   ^~~~~~~~~~~~~~~~~~~~~~
/home/hans/blender-git/blender/source/blender/blenkernel/intern/instances.cc:127:1: warning:   when initialized here [-Wreorder]
  127 | Instances::Instances(Instances &&other)
      | ^~~~~~~~~
/home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh: In copy constructor ‘blender::bke::Instances::Instances(const blender::bke::Instances&)’:
/home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh:127:35: warning: ‘blender::bke::Instances::almost_unique_ids_cache_’ will be initialized after [-Wreorder]
  127 |   mutable SharedCache<Array<int>> almost_unique_ids_cache_;
      |                                   ^~~~~~~~~~~~~~~~~~~~~~~~
/home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh:121:35: warning:   ‘blender::SharedCache<blender::Array<int> > blender::bke::Instances::reference_user_counts_’ [-Wreorder]
  121 |   mutable SharedCache<Array<int>> reference_user_counts_;
      |                                   ^~~~~~~~~~~~~~~~~~~~~~
/home/hans/blender-git/blender/source/blender/blenkernel/intern/instances.cc:137:1: warning:   when initialized here [-Wreorder]
  137 | Instances::Instances(const Instances &other)
      | ^~~~~~~~~
These are out of order ``` /home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh: In constructor ‘blender::bke::Instances::Instances(blender::bke::Instances&&)’: /home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh:127:35: warning: ‘blender::bke::Instances::almost_unique_ids_cache_’ will be initialized after [-Wreorder] 127 | mutable SharedCache<Array<int>> almost_unique_ids_cache_; | ^~~~~~~~~~~~~~~~~~~~~~~~ /home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh:121:35: warning: ‘blender::SharedCache<blender::Array<int> > blender::bke::Instances::reference_user_counts_’ [-Wreorder] 121 | mutable SharedCache<Array<int>> reference_user_counts_; | ^~~~~~~~~~~~~~~~~~~~~~ /home/hans/blender-git/blender/source/blender/blenkernel/intern/instances.cc:127:1: warning: when initialized here [-Wreorder] 127 | Instances::Instances(Instances &&other) | ^~~~~~~~~ /home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh: In copy constructor ‘blender::bke::Instances::Instances(const blender::bke::Instances&)’: /home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh:127:35: warning: ‘blender::bke::Instances::almost_unique_ids_cache_’ will be initialized after [-Wreorder] 127 | mutable SharedCache<Array<int>> almost_unique_ids_cache_; | ^~~~~~~~~~~~~~~~~~~~~~~~ /home/hans/blender-git/blender/source/blender/blenkernel/BKE_instances.hh:121:35: warning: ‘blender::SharedCache<blender::Array<int> > blender::bke::Instances::reference_user_counts_’ [-Wreorder] 121 | mutable SharedCache<Array<int>> reference_user_counts_; | ^~~~~~~~~~~~~~~~~~~~~~ /home/hans/blender-git/blender/source/blender/blenkernel/intern/instances.cc:137:1: warning: when initialized here [-Wreorder] 137 | Instances::Instances(const Instances &other) | ^~~~~~~~~ ```
JacquesLucke marked this conversation as resolved
Jacques Lucke added 1 commit 2024-07-29 20:31:48 +02:00
Iliya Katushenock reviewed 2024-07-29 20:31:54 +02:00
@ -446,0 +453,4 @@
r_data.fill(0);
const Span<int> handles = this->reference_handles();
for (const int i : IndexRange(instances_num_)) {

(const handle : handles)

`(const handle : handles)`
JacquesLucke marked this conversation as resolved
Jacques Lucke added 2 commits 2024-07-29 20:40:16 +02:00
Jacques Lucke merged commit 071b18a3cc into main 2024-07-29 20:42:25 +02:00
Jacques Lucke deleted branch spreadsheet-separate-instances 2024-07-29 20:42:28 +02:00
Member

Where would you display that exactly. Seems weird to have it next to Mesh.

It wouldn't be there. What I meant was putting the same user count number we have next to the name of the geometry set in the top tree, also next to the name in the spreadsheet for the list of instances when the instance domain is selected. (so not in the bottom tree, but in the spreadsheet itself when the instance domain is selected)

From your example, every Leaf instance would display the user count of that geometry set.

> Where would you display that exactly. Seems weird to have it next to `Mesh`. It wouldn't be there. What I meant was putting the same user count number we have next to the name of the geometry set in the top tree, also next to the name in the spreadsheet for the list of instances when the instance domain is selected. (so not in the bottom tree, but in the spreadsheet itself when the instance domain is selected) From your example, every `Leaf` instance would display the user count of that geometry set.
Author
Member

Ah, I see, will give that a try.

Ah, I see, will give that a try.
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#125293
No description provided.