Details

Reviewers

Bastien Montagne (mont29)
Sebastián Barschkis (sebbas)

Maniphest Tasks

T73201: New volume object type

Commits

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

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

Branch

new-objects-review (branched from master)

Build Status

Buildable 6815
Build 6815: arc lint + arc unit

Event Timeline

Brecht Van Lommel (brecht) created this revision.Feb 26 2020, 8:23 PM

Harbormaster completed remote builds in B6793: Diff 22101.Feb 26 2020, 8:23 PM

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).

Brecht Van Lommel (brecht) edited the summary of this revision. (Show Details)Feb 26 2020, 8:28 PM

Silas Opel (Schamph) awarded a token.Feb 26 2020, 9:47 PM

William Reynish (billreynish) awarded a token.Feb 26 2020, 11:14 PM

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

Andrew Charlton (Scaredyfish) awarded a token.Feb 27 2020, 12:00 AM

Fran Marz (franMarz) awarded a token.Feb 27 2020, 1:54 AM

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

Harbormaster completed remote builds in B6805: Diff 22116.Feb 27 2020, 9:39 AM

Philipp Oeser (lichtwerk) added a subscriber: Philipp Oeser (lichtwerk).Feb 27 2020, 9:43 AM

Minor tweaks.

Harbormaster completed remote builds in B6806: Diff 22118.Feb 27 2020, 10:05 AM

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
1646	This looks more like a fix/tweak to put in master directly?
1672	Same here
source/blender/editors/interface/interface_templates.c
721–723	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
743–745	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
721–723	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.

Address comments.

Harbormaster completed remote builds in B6815: Diff 22134.Feb 27 2020, 4:01 PM

Fix Python errors when building without hair and pointclouds

Harbormaster completed remote builds in B6824: Diff 22145.Feb 27 2020, 7:05 PM

Brecht Van Lommel (brecht) mentioned this in D6955: Objects: Eevee and workbench rendering of new Volume, Hair, PointCloud.Feb 27 2020, 7:23 PM

Brecht Van Lommel (brecht) added a child revision: D6955: Objects: Eevee and workbench rendering of new Volume, Hair, PointCloud.Feb 27 2020, 7:24 PM

Sebastián Barschkis (sebbas) added inline comments.Feb 28 2020, 12:03 PM

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.

Harbormaster completed remote builds in B6841: Diff 22176.Feb 28 2020, 3:01 PM

Brecht Van Lommel (brecht) marked 2 inline comments as done.Feb 28 2020, 3:02 PM

Sebastián Barschkis (sebbas) added inline comments.Feb 28 2020, 3:13 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.

Jason Clarke (jtheninja) added a subscriber: Jason Clarke (jtheninja).Mar 1 2020, 11:11 PM

Julien Leménicier (mistajuliax) awarded a token.Mar 2 2020, 10:00 AM

Julien Leménicier (mistajuliax) added a subscriber: Julien Leménicier (mistajuliax).

Add assert when adding empty grid names

Harbormaster completed remote builds in B6885: Diff 22275.Mar 2 2020, 4:55 PM

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

André Savik (klutz) awarded a token.Mar 5 2020, 10:57 PM

Sebastián Barschkis (sebbas) accepted this revision.Mar 9 2020, 12:52 PM

Rebase on master (no need to review again).

Harbormaster completed remote builds in B7149: Diff 22772.Mar 13 2020, 12:47 PM

Closed by commit rBb0a1cf2c9ae6: Objects: add Volume object type, and prototypes for Hair and PointCloud (authored by Brecht Van Lommel (brecht)). · Explain WhyMar 18 2020, 11:24 AM

This revision was automatically updated to reflect the committed changes.

Brecht Van Lommel (brecht) added a commit: rBb0a1cf2c9ae6: Objects: add Volume object type, and prototypes for Hair and PointCloud.

Objects: add Volume object type, and prototypes for Hair and PointCloudClosedPublicActions

Details

Diff Detail

Event Timeline

Large Diff

Revision ContentsChangeset List

Diff 22134

CMakeLists.txt

release/scripts/startup/bl_ui/__init__.py

release/scripts/startup/bl_ui/properties_data_hair.py

release/scripts/startup/bl_ui/properties_data_pointcloud.py

release/scripts/startup/bl_ui/properties_data_volume.py

release/scripts/startup/bl_ui/properties_object.py

release/scripts/startup/bl_ui/space_dopesheet.py

release/scripts/startup/bl_ui/space_filebrowser.py

release/scripts/startup/bl_ui/space_node.py

release/scripts/startup/bl_ui/space_outliner.py

release/scripts/startup/bl_ui/space_userpref.py

release/scripts/startup/bl_ui/space_view3d.py

source/blender/blenkernel/BKE_hair.h

source/blender/blenkernel/BKE_main.h

source/blender/blenkernel/BKE_packedFile.h

source/blender/blenkernel/BKE_pointcloud.h

source/blender/blenkernel/BKE_volume.h

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/anim_sys.c

source/blender/blenkernel/intern/bpath.c

source/blender/blenkernel/intern/customdata.c

source/blender/blenkernel/intern/effect.c

source/blender/blenkernel/intern/hair.c

source/blender/blenkernel/intern/idcode.c

source/blender/blenkernel/intern/lib_id.c

source/blender/blenkernel/intern/lib_id_delete.c

source/blender/blenkernel/intern/lib_query.c

source/blender/blenkernel/intern/lib_remap.c

source/blender/blenkernel/intern/main.c

source/blender/blenkernel/intern/material.c

source/blender/blenkernel/intern/object.c

source/blender/blenkernel/intern/object_update.c

source/blender/blenkernel/intern/packedFile.c

source/blender/blenkernel/intern/pointcloud.c

source/blender/blenkernel/intern/volume.cc

source/blender/blenloader/intern/readblenentry.c

source/blender/blenloader/intern/readfile.h

source/blender/blenloader/intern/readfile.c

source/blender/blenloader/intern/writefile.c

source/blender/blentranslation/BLT_translation.h

source/blender/depsgraph/CMakeLists.txt

source/blender/depsgraph/intern/builder/deg_builder_nodes.cc

source/blender/depsgraph/intern/builder/deg_builder_relations.cc

source/blender/depsgraph/intern/depsgraph_tag.cc

source/blender/depsgraph/intern/eval/deg_eval_runtime_backup.h

source/blender/depsgraph/intern/eval/deg_eval_runtime_backup.cc

source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.h

source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.cc

source/blender/editors/animation/anim_channels_defines.c

source/blender/editors/animation/anim_channels_edit.c

source/blender/editors/animation/anim_filter.c

source/blender/editors/include/ED_anim_api.h

source/blender/editors/include/UI_icons.h

source/blender/editors/interface/interface_icons.c

source/blender/editors/interface/interface_templates.c

source/blender/editors/object/CMakeLists.txt

source/blender/editors/object/object_add.c

source/blender/editors/object/object_intern.h

source/blender/editors/object/object_modifier.c

source/blender/editors/object/object_ops.c

source/blender/editors/object/object_relations.c

source/blender/editors/object/object_volume.c

source/blender/editors/render/render_opengl.c

source/blender/editors/space_buttons/CMakeLists.txt

source/blender/editors/space_buttons/buttons_context.c

source/blender/editors/space_file/filelist.c

source/blender/editors/space_file/filesel.c

source/blender/editors/space_info/info_stats.c

source/blender/editors/space_nla/nla_buttons.c

source/blender/editors/space_nla/nla_channels.c

source/blender/editors/space_node/node_edit.c

source/blender/editors/space_outliner/outliner_draw.c

source/blender/editors/space_outliner/outliner_intern.h

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

Revision Contents
Changeset List

release/scripts/startup/bl_ui/init.py