Page MenuHome

Objects: add Volume object type, and prototypes for Hair and PointCloud
ClosedPublic

Authored by Brecht Van Lommel (brecht) on Feb 26 2020, 8:23 PM.
Tags
None
Tokens
"Love" token, awarded by klutz."Love" token, awarded by mistajuliax."100" token, awarded by franMarz."Burninate" token, awarded by Scaredyfish."Love" token, awarded by Way."Like" token, awarded by billreynish."Like" token, awarded by Schamph.

Details

Summary
  • Only the volume object is exposed in the user interface. It is based on OpenVDB internally.
  • This does not include the viewport drawing or rendering code, that follows in another code review. For testing, build the new-object-types branch so that you can see the volumes.
  • Hair and PointCloud object types are hidden behind a WITH_NEW_OBJECT_TYPES build option. These data structures are not meant to be reviewed, besides their integration into the rest of the code similar to volumes. The reason they are included is since this will make it easier to cooperate on development for future release, and avoids tricky merges.
  • There is still an issue to solve with not enough bits being available for FILTER_ID used in the file browser. A separate code review will be submitted for that.
  • This code review depends on D6695 (will be automatically applied when using arc patch).

Relevant design docs:
https://wiki.blender.org/wiki/Source/Objects/New_Object_Types
https://wiki.blender.org/wiki/Source/Objects/Volume

Diff Detail

Repository
rB Blender

Event Timeline

The choice of reviewers is always a bit arbitrary for these kinds of big changes. I picked Bastien because he's the datablock export, and Sebastián since this is related to fluid simulation (even though it doesn't integrate with that yet).

Way awarded a token.Feb 26 2020, 11:47 PM

Remove drawing code and icons which don't need to be reviewed here.

From just reading the code, ID management side of things looks fine, besides few minor notes below (the main one being about indices of the new IDtypes).

source/blender/blenkernel/intern/hair.c
101–103

This is not really needed? unless you want to be fully explicit here...

Also, shouldn't this use the new DNA default system?

MEMCPY_STRUCT_AFTER(hair, DNA_struct_default_get(Hair), id);

(same goes for the two other types' _init() functions)

source/blender/blenkernel/intern/material.c
1650

This looks more like a fix/tweak to put in master directly?

1676

Same here

source/blender/editors/interface/interface_templates.c
719–721

Would need to check again, also because it has been a long time since this changed, but iirc this was split in two 'calls' because parsing code in i18n messages extraction tools was not able to deal with more than a certain number of 'parameters'... Could be an occasion to add a third 'call' and origanize them a bit more logically?

source/blender/makesdna/DNA_ID.h
761–763

This index is used as order of processing in many areas of ID management (through set_listbasepointers()), I would rather have them right after the other obdata pointers (especially before ID_OB index).

Brecht Van Lommel (brecht) marked 5 inline comments as done.Feb 27 2020, 4:01 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/editors/interface/interface_templates.c
719–721

I added a comment and checked it's within limits now.

I rather do not change the order of other dataypes in this patch. I did that in an earlier patch and it caused many merge issues. We can sort this and other places alphabetically after this is in master.

Brecht Van Lommel (brecht) marked an inline comment as done.

Address comments.

Fix Python errors when building without hair and pointclouds

source/blender/blenkernel/intern/volume.cc
707

Same as previous comment on BKE_volume_load().

source/blender/editors/object/object_volume.c
117

Shouldn't the return value of BKE_volume_load() be considered too? If it fails because a file does not exist, imported would still be set to true.

Address comments regarding BKE_volume_load.

Brecht Van Lommel (brecht) marked 2 inline comments as done.Feb 28 2020, 3:02 PM
source/blender/blenkernel/intern/volume.cc
1050

If the transform is checked here, shouldn't then there also be a vdb_grid->setTransform() in BKE_volume_grid_add()?

1129

It would be nice if there was a way to distinguish between regular and staggered float vector grids. Right now the distinction is just by name.

For example, for a fluid velocity grid I would want to do vdb_grid->setGridClass(openvdb::GRID_STAGGERED).

This might be considered a feature request though :)

1133

Not sure if it should be allowed to add a grid with an empty name (name is ""). Maybe this should be prevented here too?

Brecht Van Lommel (brecht) marked 2 inline comments as done.Mar 1 2020, 4:00 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenkernel/intern/volume.cc
1050

That can be set directly on the OpenVDB grid retrieved using BKE_volume_grid_openvdb_for_write().

We could make that part of the BKE_volume.h API, but I don't see a need for it. Particularly since OpenVDB has various different transform types, so the API functions wouldn't be simple.

1129

I should expose the grid class in the UI and Python API, but maybe after this patch is committed.

This information is available already when you get the OpenVDB grid though. I think we can expand the C API as needed. If there's code that needs to do a lot with an OpenVDB grid it can be written in C++ too.

1133

I don't think OpenVDB disallows grids with empty names, though indeed we should probably avoid creating those ourselves, I can add an assert here.

Brecht Van Lommel (brecht) marked 2 inline comments as done.

Add assert when adding empty grid names

As far as I am concerned (i.e. within my "domain of expertise", ID management), code now LGTM.

This revision is now accepted and ready to land.Mar 3 2020, 3:29 PM

Rebase on master (no need to review again).