Page MenuHome

Consolidate Modifier UI Drawing into MOD_*.c Files
AbandonedPublic

Authored by Hans Goudey (HooglyBoogly) on Fri, Mar 13, 7:32 AM.

Details

Summary

Similar to the way operators can define their own UI directly through defining a callback, this patch moves modifier UI drawing to a similar system.

This was described in the Extensible Architecture Proposal. The idea is that the code becomes more organized, and self contained.

There are no visible changes except for a bit of simplification to the Data Transfer modifier's UI. Apparently I'm sort of masochistic because I went and rewrote 1700 lines of UI in C. I didn't get to the grease pencil modifiers though.

From my experience it's just a little more complicated than writing it in Python, but it's more purposeful too, and really not that different. The compiler can even find some errors for you.

A uiDraw callback is added to ModifierTypeInfo.

Diff Detail

Event Timeline

Hans Goudey (HooglyBoogly) planned changes to this revision.EditedFri, Mar 13, 7:34 AM
  • Not working:
    • Texture template indented weirdly
    • object operator with different arguments (ocean modifier)
    • Cycles feature settings for subsurf
    • Projectors in UV project modifier
  • Use translations (IFACE_?)
  • Make sure to use const

Thanks!

This extensible architecture proposal has not been officially approved yet, though I know Bastien and Sergey agree with it. @Campbell Barton (campbellbarton), do you have any concerns about this part?

source/blender/editors/interface/interface_templates.c
2098–2101

It should be allowed for this mti->uiDraw to be NULL.

source/blender/modifiers/intern/MOD_bevel.c
278–280

bool has_vertex_group = RNA_string_length(&ptr, "vertex_group") != 0;

source/blender/modifiers/intern/MOD_ocean.c
596

UNUSED_VARS(ptr);

Hans Goudey (HooglyBoogly) marked 3 inline comments as done.
  • Use translation
  • Add UNUSED_VARS
  • Allow uiDraw to be NULL
  • Use a simpler method for finding has_vertex_group
Hans Goudey (HooglyBoogly) planned changes to this revision.Fri, Mar 13, 7:37 PM

Not sure whether to use const for bContext, uiTemplateID and uiTemplateCacheFile complain about it.

Still have to figure out these:

  • Texture template indented weirdly
  • Operators with different arguments (ocean modifier)
  • Cycles feature settings for subsurf modifier
  • Projectors in UV project modifier

Looks good!

Some of the checks using RNA could access the data directly, eg:

bool has_vertex_group = RNA_string_length(&ptr, "vertex_group") != 0;

Could be:

bool has_vertex_group = mod->vgroup[0] != '\0';

Is there a good reason to disallow accessing the modifier data directly?

The layout code could end up roughly matching the UI drawing in source/blender/editors/space_node/drawnode.c, where RNA is used for the defining the UI and the modifiers can be read from - since RNA isn't necessary in this case.

source/blender/blenkernel/BKE_modifier.h
342–343

Any reason PointerRNA needs to be passed in by value instead of a pointer?

Is there a good reason to disallow accessing the modifier data directly?

I felt that it was simplest to just use RNA for accessing these properties for a few reasons:

  1. Keeps the changes from the previous implementation of the UI minimal-- much easier for anyone who has worked on the modifier's UI (and faster for me making this change, it took a few days anyway without digging through the ModifierData structs)
  2. Just using one system is simpler-- if I access some properties directly and others through RNA it could be confusing.
  3. I feel that using the higher level of abstraction makes more sense for UI code.
  4. It enables more consistency across the UI for all the modifiers, which was something I focused on for this patch. I think in general DNA values can have pretty cryptic names.

I changed to using pointers to the PointerRNAs for the modifier data and the object though.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Use pointers to PointerRNA for uiDraw
  • Use arguments to object operators
  • Add ocean modifier spectrum settings from recent commit
Hans Goudey (HooglyBoogly) marked an inline comment as done.Tue, Mar 17, 5:54 PM

Here are the last things I need to figure out:

I'd appreciate any help here.

source/blender/modifiers/intern/MOD_subsurf.c
302

Here I'm not sure how to access the cycles properties properly.

Here was the Python:

show_adaptive_options = (
    engine == 'CYCLES' and md == ob.modifiers[-1] and
    scene.cycles.feature_set == 'EXPERIMENTAL'
)

and

ob.cycles.use_adaptive_subdivision
source/blender/modifiers/intern/MOD_uvproject.c
338

How to list the projector's objects here:

The python was simple:

for proj in md.projectors:
      col.prop(proj, "object", text="")

But I'm not sure of the best / most readable way to do this here.

source/blender/modifiers/intern/MOD_subsurf.c
302

I think you should be able to access them through the RNA API?

if (BKE_scene_uses_cycles(scene)) {
    PointerRNA cycles_ptr = RNA_pointer_get(&sceneptr, "cycles");
    ...
}
source/blender/modifiers/intern/MOD_uvproject.c
338

RNA_BEGIN / RNA_END probably works here.

@Brecht Van Lommel (brecht) We're in Bcon2 now right? Can I finish this up now or will it have to wait for next cycle?

This can be done in Bcon2 if it's relatively safe. I haven't read through the patch yet to see what the risk is. If not committing can wait a few weeks and go into master when that reopens.

IMO the biggest risk is that I set some "uiLayoutEnabled" flag wrong and it doesn't look quite right. I tested thoroughly but it's a lot of lines. Stuff that can be easily fixed if it happens to come up anyway.

It's not too bad to wait, especially through Bcon2 where no new modifier properties will be added.

I'm also starting a temp-modifier-ui branch with @William Reynish (billreynish) looking to solve some of the longer-standing issues with modifier UI. Since this patch seems to be positively received it's going into that branch anyway.

After discussion with @William Reynish (billreynish), this turned into a bigger project that I'm working on in the modifier-panels-ui branch. I'll make another revision when that's finished, but It's not worth it to keep both changes alive at once.