Refactor: UI: Make uiItem layout hierarchy use C++ inheritance. #124405

Merged
Bastien Montagne merged 4 commits from mont29/blender:tmp-ui-layout-cpp-allocation into main 2024-07-10 14:46:25 +02:00

This commit turns the base struct uiItem and all of its descendants
(including uiButtonItem, uiLayout`, etc.) into a C++ polymorphic
hierarchy of types.

This allows to use C++ type of memory management, and use non-trivial
types (which will be required to make PointerRNA non-trivial).

It also moves the storage of these uiItems from BLI_listbase to
blender::Vector, as our C-based listbase implementation is
incompatible with C++ polymorphism.

This also lead to making uiItem parameters of a few utils functions
const, to allow passing around blender::Span instead of vectors to
some internal helpers.

This commit turns the base struct `uiItem` and all of its descendants (including `uiButtonItem`, uiLayout`, etc.) into a C++ polymorphic hierarchy of types. This allows to use C++ type of memory management, and use non-trivial types (which will be required to make `PointerRNA` non-trivial). It also moves the storage of these `uiItems` from `BLI_listbase` to `blender::Vector`, as our C-based listbase implementation is incompatible with C++ polymorphism. This also lead to making `uiItem` parameters of a few utils functions `const`, to allow passing around `blender::Span` instead of vectors to some internal helpers.
Bastien Montagne added 1 commit 2024-07-09 13:06:37 +02:00
UI Layout: Refactor: Make uiItem layout hierarchy use C++ inheritance.
Some checks failed
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
c470ed0097
This commit turns the base struct `uiItem` and all of its descendants
(including `uiButtonItem`, uiLayout`, etc.) into a C++ polymorphic
hierarchy of types.

This allows to use C++ type of memory management, and use non-trivial
types (which will be required to make `PointerRNA` non-trivial).

It also moves the storage of these `uiItems` from `BLI_listbase` to
`blender::Vector`, as our C-based listbase implementation is
incompatible with C++ polymorphism.

This also lead to making `uiItem` parameters of a few utils functions
`const`, to allow passing around `blender::Span` instead of vectors to
some internal helpers.
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne requested review from Hans Goudey 2024-07-09 13:07:12 +02:00
Bastien Montagne requested review from Julian Eisel 2024-07-09 13:07:21 +02:00
Hans Goudey changed title from UI Layout: Refactor: Make `uiItem` layout hierarchy use C++ inheritance. to Refactor: UI: Make `uiItem` layout hierarchy use C++ inheritance. 2024-07-09 14:00:01 +02:00
Hans Goudey requested changes 2024-07-09 14:32:49 +02:00
Dismissed
Hans Goudey left a comment
Member

So much nicer! Thanks for doing this.

So much nicer! Thanks for doing this.
@ -146,3 +146,3 @@
bContextStore *context;
uiLayout *parent;
ListBase items;
blender::Vector<uiItem *> items;
Member

A vector of unique_ptr would be even nicer I think. But okay to do that as a separate step.
Can remove // ListBase items; though.

A vector of `unique_ptr` would be even nicer I think. But okay to do that as a separate step. Can remove `// ListBase items;` though.
Author
Owner

Indeed moving to unique_ptr would be a good next step - but think this PR is changing more than enough already 😅

Indeed moving to `unique_ptr` would be a good next step - but think this PR is changing more than enough already 😅
mont29 marked this conversation as resolved
@ -375,3 +366,3 @@
{
if (item->type == ITEM_BUTTON) {
uiButtonItem *bitem = (uiButtonItem *)item;
const uiButtonItem *bitem = dynamic_cast<const uiButtonItem *>(item);
Member

dynamic_cast is typically much slower than reinterpret_cast. 5-30 times slower according to https://stackoverflow.com/a/7579250/10444036

Conceptually it's also redundant with the item->type check. If the item type doesn't match the actual type, we have much larger problems, so dynamic_cast is not really making this safer. So I think it would be better to use reinterpret_cast in this PR.

`dynamic_cast` is typically much slower than `reinterpret_cast`. 5-30 times slower according to https://stackoverflow.com/a/7579250/10444036 Conceptually it's also redundant with the `item->type` check. If the item type doesn't match the actual type, we have much larger problems, so `dynamic_cast` is not really making this safer. So I think it would be better to use `reinterpret_cast` in this PR.
Author
Owner

I do not agree here... As pointed out in your stackoverflow thread, comparing dynamic_cast to reinterpret_cast is useless, since the later usually does not do anything. In that sense, dynamic_cast is infinitely slower than reinterpret_cast!

And I think that the point of using C++ is also to have safer, saner, less heavy-maintenance code. dynamic_cast helps here, and can e.g. make catching invalid type casting caused by some random bug way easier.

I don't see the point of sacrificing simplicity, maintainability, reliability and 'correctness' of code for nano-optimizations, unless there are evidences that the affected code is a bottleneck in performances.

I do not agree here... As pointed out in your stackoverflow thread, comparing `dynamic_cast` to `reinterpret_cast` is useless, since the later usually does not do anything. In that sense, `dynamic_cast` is _infinitely_ slower than `reinterpret_cast`! And I think that the point of using C++ is also to have safer, saner, less heavy-maintenance code. `dynamic_cast` helps here, and can e.g. make catching invalid type casting caused by some random bug way easier. I don't see the point of sacrificing simplicity, maintainability, reliability and 'correctness' of code for nano-optimizations, unless there are evidences that the affected code is a bottleneck in performances.
Member

dynamic_cast is useful when you don't already store the type separately. It's redundant with item->type, so it's just misleading and wasteful.

dynamic_cast isn't simpler, and the only thing that's made "safer" is if the type is mismatched with the object type. That's not going to be a problem here.

Not wasting time on small optimizations doesn't mean needlessly introducing overhead and redundancy. I would personally go with std::variant here over dynamic_cast.

`dynamic_cast` is useful when you don't already store the type separately. It's redundant with `item->type`, so it's just misleading and wasteful. `dynamic_cast` isn't simpler, and the only thing that's made "safer" is if the type is mismatched with the object type. That's not going to be a problem here. Not wasting time on small optimizations doesn't mean needlessly introducing overhead and redundancy. I would personally go with `std::variant` here over `dynamic_cast`.
Bastien Montagne added 1 commit 2024-07-09 14:58:14 +02:00
Some fixes and cleanups.
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
53e58bf14a
Author
Owner

@blender-bot build

@blender-bot build
Author
Owner

So, talked to half a dozen devs in the team, and got about half recommending to use dynamic_cast, the other half recommending to use static_cast.

I would still rather stick to dynamic_cast personally, because the added cost is not an issue here, and it add some form of validation that the code handling the uiItem->type data is not broken.

But for the time being will switch the patch to static_cast, since this is essentially what the previous C code was doing.

So, talked to half a dozen devs in the team, and got about half recommending to use `dynamic_cast`, the other half recommending to use `static_cast`. I would still rather stick to `dynamic_cast` personally, because the added cost is not an issue here, and it add some form of validation that the code handling the `uiItem->type` data is not broken. But for the time being will switch the patch to `static_cast`, since this is essentially what the previous C code was doing.
Bastien Montagne added 1 commit 2024-07-10 10:36:16 +02:00
Bastien Montagne added 1 commit 2024-07-10 10:36:57 +02:00
Switch from dynamic_cast to static_cast
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
c2c356fa8c
Author
Owner

@blender-bot build

@blender-bot build
Hans Goudey approved these changes 2024-07-10 13:49:46 +02:00
Hans Goudey left a comment
Member

Thanks! Looks great to me now.

Thanks! Looks great to me now.
Bastien Montagne merged commit a3f0d81a5e into main 2024-07-10 14:46:25 +02:00
Bastien Montagne deleted branch tmp-ui-layout-cpp-allocation 2024-07-10 14:46:30 +02:00
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
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
FBX
Interest
Freestyle
Interest
Geometry Nodes
Interest
glTF
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 & 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
Asset System
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline & 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
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#124405
No description provided.