More flexible callback for persistent panel expand flags #108648

Closed
Lukas Tönne wants to merge 2 commits from LukasTonne/blender:panel_expand_flags_bit_vector into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

"Expand flags" are persistent storage that helps Blender remember the open/closed state of panels, even when the panels themselves are destroyed (e.g. on workspace changes). These flags are stored in various places where panels are created (e.g. modifiers) and currently exposed to the panel system via get/set callback.

So far the callback supports up to 16 bits by returning a short by value. This is fine for existing fixed panel layouts, but for future dynamic panels (node modifier socket sub-panels) this is a significant limitation. To make more flag bits possible the callback now supports returning an arbitrary array of bits as a uint64_t *.

This simplifies the internal code too, because we can just use a BitIterator to loop over bits in sequence instead of using offsets and masks. It's the responsibility of the panel creating code to ensure there are enough bits for the number of child panels generated. For now all panel types return a pointer to a short as before, but now a larger type could be used, including a full BitVector if needed.

"Expand flags" are persistent storage that helps Blender remember the open/closed state of panels, even when the panels themselves are destroyed (e.g. on workspace changes). These flags are stored in various places where panels are created (e.g. modifiers) and currently exposed to the panel system via get/set callback. So far the callback supports up to 16 bits by returning a `short` by value. This is fine for existing fixed panel layouts, but for future dynamic panels (node modifier socket sub-panels) this is a significant limitation. To make more flag bits possible the callback now supports returning an arbitrary array of bits as a `uint64_t *`. This simplifies the internal code too, because we can just use a BitIterator to loop over bits in sequence instead of using offsets and masks. It's the responsibility of the panel creating code to ensure there are enough bits for the number of child panels generated. For now all panel types return a pointer to a `short` as before, but now a larger type could be used, including a full `BitVector` if needed.
Lukas Tönne added 2 commits 2023-06-06 10:38:55 +02:00
Lukas Tönne requested review from Hans Goudey 2023-06-06 10:39:05 +02:00
Lukas Tönne added this to the Nodes & Physics project 2023-06-06 10:39:09 +02:00
Hans Goudey reviewed 2023-06-06 14:02:37 +02:00
Hans Goudey left a comment
Member

Mostly looks good! I think returning the integer via pointer doesn't really get us anything at the moment-- it still gives a 64 panel limit. That's fine for now I guess, so we might as well return a uint64_t directly.

In the future it would be nice to return BitVector, or an allocated array of uint64_t.

Mostly looks good! I think returning the integer via pointer doesn't really get us anything at the moment-- it still gives a 64 panel limit. That's fine for now I guess, so we might as well return a `uint64_t` directly. In the future it would be nice to return `BitVector`, or an allocated array of `uint64_t`.
@ -289,15 +289,9 @@ typedef struct PanelType {
* data item. Called on draw updates.
* \note Sub-panels are indexed in depth first order,
* the visual order you would see if all panels were expanded.
* \return Pointer to bit array with at least one bit for the panel and for each child panel.
Member

at least one bit is a bit confusing. I think it means "the array is big enough, don't worry!" but it sounds like some panels might get two bits for some reason.

`at least one bit` is a bit confusing. I think it means "the array is big enough, don't worry!" but it sounds like some panels might get two bits for some reason.
Author
Member

I think returning the integer via pointer doesn't really get us anything at the moment-- it still gives a 64 panel limit

Yes, it's in preparation for returning a larger bit vector for modifier panels. I used uint64_t because it's the storage type used for BitVector/BitIterator although it's quite arbitrary, could even return void *. The main difference is that we expect this to be a contiguous array, so the iterator can go beyond the initial integer. BitIterator takes care of this.

In the future it would be nice to return BitVector

Agreed. But this is still a C callback. Having the additional size property would give us a way to assert that there are enough bits for all the panels. So far we just assume that the panel creator code knows to reserve enough bits.

> I think returning the integer via pointer doesn't really get us anything at the moment-- it still gives a 64 panel limit Yes, it's in preparation for returning a larger bit vector for modifier panels. I used `uint64_t` because it's the storage type used for `BitVector`/`BitIterator` although it's quite arbitrary, could even return `void *`. The main difference is that we expect this to be a _contiguous_ array, so the iterator can go beyond the initial integer. `BitIterator` takes care of this. > In the future it would be nice to return BitVector Agreed. But this is still a C callback. Having the additional size property would give us a way to assert that there are enough bits for all the panels. So far we just assume that the panel creator code knows to reserve enough bits.
Member

Sure, I'm just not sure that returning a pointer instead of a integer value helps prepare for BitVector more. Maybe until we can make this a C++ callback, it's better to use the value? It's also just a bit weird to have a uint64_t pointer that points to a short.

Sure, I'm just not sure that returning a pointer instead of a integer value helps prepare for `BitVector` more. Maybe until we can make this a C++ callback, it's better to use the value? It's also just a bit weird to have a `uint64_t` pointer that points to a `short`.
Author
Member

Sure, I'm just not sure that returning a pointer instead of a integer value helps prepare for BitVector more. Maybe until we can make this a C++ callback, it's better to use the value? It's also just a bit weird to have a uint64_t pointer that points to a short.

Would a void * be better? We don't really care about the underlying data type. Returning a void * at least does not suggest that the data has any specific size (other than "large enough for all panels").

Returning bits by value would also require a setter function again, at which point PR would be pointless. Or is it pointerless?

Could also return pointer + size, and then we construct a BitSpan locally in interface_panel.cc. Once PanelType supports C++ we can swap this out for a BitSpan in the callback itself.

static void get_modifier_expand_flag(const bContext * /*C*/, Panel *panel, void **r_bits, int *r_size)
> Sure, I'm just not sure that returning a pointer instead of a integer value helps prepare for `BitVector` more. Maybe until we can make this a C++ callback, it's better to use the value? It's also just a bit weird to have a `uint64_t` pointer that points to a `short`. Would a `void *` be better? We don't really care about the underlying data type. Returning a `void *` at least does not suggest that the data has any specific size (other than "large enough for all panels"). Returning bits by value would also require a setter function again, at which point PR would be pointless. Or is it pointerless? Could also return pointer + size, and then we construct a `BitSpan` locally in interface_panel.cc. Once PanelType supports C++ we can swap this out for a BitSpan in the callback itself. ```cpp static void get_modifier_expand_flag(const bContext * /*C*/, Panel *panel, void **r_bits, int *r_size) ```
Member

Either way, without split get and set callbacks, BitVector doesn't really work at all for the future, I think that's why I was confused.

I don't think void * would be better, it just removes all type information. If the situation feels better to you I'm fine with this change, but including the downsides, this doesn't really feel like an improvement to me personally. I guess it's better if I think about it as a first step to using BitSpan.

Either way, without split `get` and `set` callbacks, `BitVector` doesn't really work at all for the future, I think that's why I was confused. I don't think `void *` would be better, it just removes all type information. If the situation feels better to you I'm fine with this change, but including the downsides, this doesn't really feel like an improvement to me personally. I guess it's better if I think about it as a first step to using `BitSpan`.
Author
Member

Ok, i'm dropping it then, can revisit when we can actually do this in C++.

Ok, i'm dropping it then, can revisit when we can actually do this in C++.
Lukas Tönne closed this pull request 2023-06-06 18:15:33 +02:00

Pull request closed

Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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 Assignees
2 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#108648
No description provided.