Page MenuHome

UI: Make uiBut safe for non-trivial construction
ClosedPublic

Authored by Julian Eisel (Severin) on Tue, Jan 31, 5:26 PM.

Details

Summary

Essentially, this makes it possible to use C++ types like std::function
inside uiBut. This has plenty of benefits, for example this should
help significantly reducing unsafe void * use (since a
std::function can hold arbitrary data while preserving types).


I wanted to use a non-trivially-constructible C++ type (std::function)
inside uiBut. But this would mean we can't use MEM_cnew()
like allocation anymore.

Rather than writing worse code, allow non-trivial construction for
uiBut. Member-initializing all members is annoying since there are so
many, but rather safe than sorry. As we use more C++ types (e.g. convert
callbacks to use std::function), this should become less since they
initialize properly on default construction.

Also use proper C++ inheritance for uiBut subtypes, the old way to
allocate based on size isn't working anymore.

Diff Detail

Repository
rB Blender

Event Timeline

Julian Eisel (Severin) requested review of this revision.Tue, Jan 31, 5:26 PM
Julian Eisel (Severin) created this revision.
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Tue, Jan 31, 5:29 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)
  • Cleanup: Remove unnecessary macro & unnecessary cast
  • Fix assert failures because of unexpected RNA index default

Great cleanup, I'm glad it's possible now. Longer term I'm a bit skeptical of adding more cases of storing arbitrary data for callbacks in buttons (seems the system for that is RNA), but there's a long road ahead before we get there. Better C++ polymorphism instead of this mess is an obvious first step.

source/blender/editors/include/UI_interface.hh
22 ↗(On Diff #60089)

Is this needed? Doesn't seem like it from the rest of the header

source/blender/editors/interface/interface.cc
3981

Best to get rid of this function soon-- storing eButType is redundant with the polymorphism here. But this is an obvious change for now, one step at a time.

This revision is now accepted and ready to land.Wed, Feb 1, 5:44 PM
Julian Eisel (Severin) marked an inline comment as done.Fri, Feb 3, 4:12 PM

Longer term I'm a bit skeptical of adding more cases of storing arbitrary data for callbacks in buttons (seems the system for that is RNA), but there's a long road ahead before we get there.

Although I agree, we shouldn't rely too much on callbacks and storing arbitrary data for them, I don't think RNA is the right system for that. It's not reliable, performant or flexible enough. The UI often needs very specific behavior that RNA is not designed for.

source/blender/editors/interface/interface.cc
3981

Agreed but then we need some other kind of factory function. E.g. something like UI_but_new<uiButNumber>(...) - which would be either a big change or a 3rd API to create buttons.

source/blender/editors/interface/interface.cc
3981

I'm not sure any factory function is necessary. Nothing wrong with simply std::make_unique<uiButNumber> IMO

Jacques Lucke (JacquesLucke) added inline comments.
source/blender/editors/interface/interface.cc
3477

Only checking this on my phone right now, so didn't try. It feels like this might be doing the wrong thing for the subclasses of uiBut, because it only calls the destructor of the base class. Am I missing something?

source/blender/editors/interface/interface.cc
3477

There’s currently no virtual function and no virtual destructor. But I guess we should add one already so subclasses can use non-trivially destructible types.