Page MenuHome

Geometry Nodes: Use separate field contexts for different geometry types
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Jul 22 2022, 10:40 PM.

Details

Summary

Using the same GeometryComponentFieldContext for all situations,
even when only one geometry type is supported is misleading, and mixes
too many different abstraction levels into code that could be simpler.
With the attribute API moved out of geometry components recently,
the "component" system is just getting in the way here.

This commit adds specific field contexts for geometry types: meshes,
curves, point clouds, and instances. There are also separate field input
helper classes, to help reduce boilerplate for fields that only support
specific geometry types.

Another benefit of this change is that it separates geometry components
from fields, which makes it easier to see the purpose of the two concepts,
and how they relate.

Because we want to be able to evaluate a field on just CurvesGeometry
rather than the full Curves data-block, the generic "geometry context"
had to be changed to avoid using GeometryComponent, since there is
no corresponding geometry component type. The resulting void pointer
is ugly, but only turns up in three places in practice. When Apple clang
supports std::variant, that could be used instead.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Jul 22 2022, 10:40 PM
Hans Goudey (HooglyBoogly) created this revision.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Solve the generic geometry field context issue, final cleanups

Looks thorough.

source/blender/blenkernel/BKE_geometry_fields.hh
29–31

Does it make sense to check if a domain is supported?

Add asserts for supported attribute domains

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Merge master

Jacques Lucke (JacquesLucke) requested changes to this revision.Aug 29 2022, 6:10 PM
  • Debug builds don't compile right now.
  • The output of the flower shop scene changed.

master


patch

source/blender/blenkernel/BKE_geometry_fields.hh
106

typo (CurvesGeometry)

136

I should mention that I'm not 100% sure that we'll never need a Curves * here. We may need to access Curves specific data at some point. For now just using CurvesGeometry is fine though.

source/blender/geometry/GEO_resample_curves.hh
18

Think it's fine to use using bke::CurvesGeometry at the top here.

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc
73

If a newline is after every line, it doesn't really help readability.

source/blender/nodes/geometry/nodes/node_geo_curve_reverse.cc
27

Add using::CurvesGeometry in NOD_geometry_exec.hh.

31

Not sure if that change is really an improvement over the old code.

This revision now requires changes to proceed.Aug 29 2022, 6:10 PM
Hans Goudey (HooglyBoogly) marked 6 inline comments as done.Aug 29 2022, 9:32 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenkernel/BKE_geometry_fields.hh
136

Agreed. Some field that depends on selection could already do that. If that's needed, I think the next step is to use a specific type enum to this general field context instead of GeometryComponentType.

source/blender/geometry/GEO_resample_curves.hh
18

This actually doesn't seem to compile with using blender::bke::CurvesGeometry at the top of the file. The compiler doesn't figure out that we mean the bke type instead of the DNA type (example below).
I'm fine with that personally :P I can test more if you'd like though.

/node_geo_curve_resample.cc:94:74: error: conversion from ‘CurvesGeometry’ to non-scalar type ‘blender::bke::CurvesGeometry’ requested

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc
73

Yeah, fair enough. My goal was to separate retrieving inputs, doing the calculation, and storing the output visually. But if each step is just one or two lines I guess it isn't helpful.

source/blender/nodes/geometry/nodes/node_geo_curve_reverse.cc
31

Good call, I removed CurveComponent now.

Hans Goudey (HooglyBoogly) marked 4 inline comments as done.

Fixes, cleanup

Add using bke::CurvesGeometry; in the resample header

This revision is now accepted and ready to land.Aug 30 2022, 12:47 PM