Introduce renderer-specific properties #459

tribal-tec · 2018-07-10T13:58:52Z

new web API: set-/get-renderer-params which returns the properties of the
current renderer
hardcoded renderer types are gone, new renderers are identified by string
new PropertyObject base class of Renderer keeps properties per type
only one single renderer exists in Brayns, type switching happens on the
OSPRay level only
resolved cyclic dependency scene <-> renderer

rdumusc · 2018-07-11T08:55:07Z

brayns/common/PropertyMap.h

        else
-            property->setData(newProperty.getData());
+            property->_data = newProperty._data;


why not use property->set() or call updateProperty() instead of playing with the internals? This is even unsafe and can cause the _data to no longer match its type

rdumusc · 2018-07-11T08:56:51Z

brayns/common/PropertyMap.h

        const Type type;

+        template <typename T>
+        Type getType();


is this not redundant with the public Type type??

rdumusc · 2018-07-11T09:03:40Z

brayns/common/PropertyMap.h

+    {
+        auto property = findProperty(name);
+        if (property)
+            return property->type;


These type of statements can be rewritten as:

if (auto property = findProperty(name)) return property->type;

It improves scoping and is shorter to read

rdumusc · 2018-07-11T09:06:44Z

brayns/Brayns.cpp

+        aoStrength -= 0.1f;
+        if (aoStrength < 0.f)
+            aoStrength = 0.f;
+        renderer.updateProperty("aoWeight", aoStrength);


I think string should be defined as constants in a single location

Strictly speaking, those actions should not be in the core, hence it becomes client code where those strings are either hardcoded or better, are taken from the schema. This is the case for the web clients. This change just tries to avoid breaking existing behavior. I can define a string constant here, but it's still wrong :)

rdumusc · 2018-07-11T09:13:26Z

brayns/common/PropertyMap.h

-    PropertyMap::Property::Type getType();
-
-    Property *findProperty(const std::string &name)
+    Property *findProperty(const std::string &name) const


the alignement of * and & is wrong in multiple places. Set DerivePointerBinding: false in .clang-format to prevent that.

rdumusc · 2018-07-11T09:23:19Z

brayns/common/PropertyMap.h

+    }
+
+    /** @return the type of the given property name. */
+    Property::Type getPropertyType(const std::string &name)


rdumusc · 2018-07-11T09:41:07Z

brayns/common/PropertyObject.h

+    }
+
+private:
+    std::string _type;


rename _type -> _currentType?

rdumusc · 2018-07-11T09:48:21Z

brayns/common/PropertyObject.h

+/**
+ * Maps generic properties to user-defined types and tracks the current type
+ * for querying, setting and updating its properties.
+ */


I don't understand what "types" in this class refer to... The PropertyMap are collections of heterogeneous types, but it seems like it's used with a different meaning here. Can this be renamed / clarified?

rdumusc · 2018-07-11T10:02:15Z

plugins/RocketsPlugin/jsonPropertyMap.h

+}
+
+template <typename T, int S>
+void _addArrayPropertySchema(const std::shared_ptr<PropertyMap::Property> prop,


can't just const PropertyMap::Property& prop be used here?

rdumusc · 2018-07-11T11:35:36Z

tests/perf/renderingSettings.cpp

-    params.getRenderingParameters().setShadowIntensity(1.f);
+    auto props = renderer.getPropertyMap();
+    props.updateProperty("shadows", 1.f);
+    renderer.updateProperties(props);


I haven't been part of the technical discussions, but this first usage example does not convince me that the change is an improvement:

The code is longer

Addressing by string is error-prone

I'm not sure if type safety is guaranteed; I have the impression that props.updateProperty("shadows", "no_a_number"); would compile just fine, or am I wrong and that's the reason that I can see no test for that?

It seem to me that internal properties of the Ospray shaders are being exposed directly throughout the application all the way to the web interface, completely breaking the encapsulation. If a shader in Ospray changes, all user code that relies on that magic string has to be fixed / recompiled / redeployed.

That's the entire point: custom shaders have custom attributes, so they can and should no longer be hardcoded in the core. If the shader changes, it should be directly reflected on the client side (btw, the shader will not change really, but there will be new shaders from any plugin/module).

With the schema in place, having the properties for a specific shader being exposed is doable for the clients. Used here w/o any schema checks which tells the valid names and types, this looks wrong, agreed.

I addressed point 3, where the updated value must have the same type that the property was registered with.

OK I see... if the shaders are moved to plugins then then PropertyMap is a way to provide reflexion for arbitrary C++ classes... in that case the string keys make sense. Then yes, make sure the intended usage is well tested:
PropertyMap is filled by plugin and only existing properties can be updated by clients in a type safe-manner.

rdumusc

+1

* new web API: set-/get-renderer-params which returns the properties of the current renderer * hardcoded renderer types are gone, new renderers are identified by string * new PropertyObject base class of Renderer keeps properties per type * only one single renderer exists in Brayns, type switching happens on the OSPRay level only * resolved cyclic dependency scene <-> renderer

tribal-tec force-pushed the renderer branch from 3a331e4 to 2d75a11 Compare July 10, 2018 14:16

tribal-tec requested review from favreau and rdumusc July 10, 2018 14:22

tribal-tec force-pushed the renderer branch from 2d75a11 to 4085a25 Compare July 11, 2018 08:05

rdumusc reviewed Jul 11, 2018

View reviewed changes

tribal-tec force-pushed the renderer branch from 4085a25 to ca0facd Compare July 11, 2018 12:35

rdumusc approved these changes Jul 11, 2018

View reviewed changes

tribal-tec force-pushed the renderer branch 2 times, most recently from 38871dc to 26659d7 Compare July 12, 2018 08:35

tribal-tec force-pushed the renderer branch from 26659d7 to ac2e5ce Compare July 12, 2018 08:42

tribal-tec merged commit f5a1184 into BlueBrain:master Jul 12, 2018

tribal-tec deleted the renderer branch July 12, 2018 09:10

tribal-tec added enhancement breaking labels Sep 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce renderer-specific properties #459

Introduce renderer-specific properties #459

tribal-tec commented Jul 10, 2018

rdumusc Jul 11, 2018

rdumusc Jul 11, 2018

rdumusc Jul 11, 2018

rdumusc Jul 11, 2018

tribal-tec Jul 11, 2018

rdumusc Jul 11, 2018

rdumusc Jul 11, 2018

rdumusc Jul 11, 2018

rdumusc Jul 11, 2018

rdumusc Jul 11, 2018

rdumusc Jul 11, 2018

tribal-tec Jul 11, 2018

rdumusc Jul 11, 2018

rdumusc left a comment

Introduce renderer-specific properties #459

Introduce renderer-specific properties #459

Conversation

tribal-tec commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdumusc left a comment

Choose a reason for hiding this comment