Automated entity property serialization

[HifiExperiments]

depends on #1085, diff here: https://github.com/HifiExperiments/overte/compare/loadPriority...HifiExperiments:overte:entityProps?expand=1

thanks to @ksuprynowicz for help with cmake!

have you ever wanted to add a new entity property but you've been intimidated by all of the necessary boilerplate and different files you need to modify? now, you just have to add a single line and all of the setup will be done for you! I probably should have done this before I added a bunch of new properties 😓

all properties (except group properties) are now defined in EntityItemProperties.txt, which supports the following schema:

• a single word with no spaces or commas: delimits a section of properties, e.g. "Physics" or "Scripts". after "Common", each line delimits a type of entity (e.g. "Shape", "Zone", "Web")
• a space separated list of the following properties, ending in a comma:
  • required:
    • enum:<the c++ prop enum (not including the PROP_ prefix)>
    • prop:<the actual property name>
    • type:<the property type>
    • default:<the default value>
  • optional values:
    • min:<the min value>
    • max:<the max value>
    • fromScriptType:<the type to use when converting from script values, if different than the normal type>
    • proxy:<if this is a proxy for another property, the name of the actual property>
    • proxyType:<for a proxy prop, the type of the original property, if different from this prop's type>
    • getter:<the name of the function to use when getting this property in EntityItemProperties, if different than the default>
    • setter:<the name of the function to use when setting this property in EntityItemProperties, if different than the default>
    • networkGetter:<the expression to use when appending this property to the network stream in EntityItemProperties>
    • variableNetworkGetter:<the expression to use when appending this property to the network stream in XXXXEntityItem>
    • variableNetworkSetter:<the expression to use when reading this property from the network stream in XXXXEntityItem>
    • variableCopyGetter:<the expression to use when getting this property to go from XXXXEntityItem to EntityItemProperties>
    • variableCopySetter:<the expression to use when setting this property to go from EntityItemProperties to XXXXEntityItem>
    • readType:<the type to use when reading this property from the network in EntityItemProperties>
    • debugString:<the string to append to the debug dump for this property, e.g. units>
    • debugGetter:<the expression to use to get a debug representation of this property>
  • optional flags:
    • legacy: this is a legacy property, i.e. a proxy for another value
    • common: this is a common property that is shared by multiple Entity types
    • enum: this is an enum value
    • propertySetter: in EntityItemProperties, this property will implement its own setter
    • noScript: this property is not exposed to scripts
    • readOnly: this property is not settable from scripts
    • noNetwork: this property is not sent over the network
    • urlPermission: this property is a URL which should be guarded by the canViewAssetURLs permission
    • noVariableNetworkSetter: in XXXXEntityItem::readEntityDataFromBuffer/readEntitySubclassDataFromBuffer, this property will implement its own network setter (must still maintain the order though)
    • basicProp/renderProp/noGetterSetterProp/inherited: specifies how the getter + setter should be defined in XXXXEntityItem. a basicProp will define them for you and just directly store the data. a renderProp will set _needsRenderUpdate to true if the property changes. a noGetterSetterProp will define the member variable but not the getter/setter. an inherited prop will not define anything, assuming both the member variable and getter/setter are inherited. if none of these are specified, the member variable will be defined as well as the getter/setter, but you must implement the getter/setter in the .cpp file.
• group:<lowercase group property name> followed by optional properties, followed by a comma: represents a property group which uses the group name for scripts
  • properties is a space separated list which can contain any of the following:
    • common: if this is a "common" group, shared by multiple entity types
    • type:<camelcase actual type>: some groups have a different c++ file name, e.g. ring is RingGizmo. if this isn't specified, the group name is just capitalized, e.g. pulse to Pulse.
    • renderUpdateOnSet: in setSubClassProperties, set _needsRenderUpdate to true to trigger a render update if any of the group properties change
    • recordChange: in setSubClassProperties, set a member variable (e.g. _hazePropertiesChanged) to true if any of the group properties change
    • customVariableSetFrom: specifies that the class implements its own handling of how to react to changes to this group in setSubClassProperties
    • customVariableRead: specifies that the class implements its own handling of how to react to changes to this group in readEntitySubclassDataFromBuffer

all group properties are defined in EntityItemGroupProperties.txt, which supports the same schema

[HifiExperiments]

this is a bit complicated to review since the diff is almost all deletion and the actual code is generated by cmake. the way I double checked everything as I went was to run cmake locally and compare the new output for all of the files to the files before this change. most properties are super simple, but some have subtle tricky things about them that we need to make sure are maintained.

[ksuprynowicz]

I just tested it using a copy of Overte_hub, since it's a heavy world, and everything works perfectly.

[ksuprynowicz]

Would anyone else like to test it, or can we mark it as QA approved?

[HifiExperiments]

(more testing is always great but I will say that for this PR, a careful code review comparing the new generated code to the old code will probably be the most effective method of catching issues, as the bugs are likely to be extremely subtle things related to some of the more complex properties like simulation ownership)

[ksuprynowicz]

It looks like in EntityItemProperties.h LinePoints have moved from Polyline to Common section (l.235):

DEFINE_PROPERTY_REF(LinePoints, linePoints, qVectorVec3, ENTITY_ITEM_DEFAULT_EMPTY_VEC3_QVEC);

[ksuprynowicz]

In EntityItemProperties.h shape property definition has changed from QString to EntityShape - is it intended?

	// Shape
	DEFINE_PROPERTY_REF_ENUM(Shape, shape, EntityShape, EntityShape::Sphere);

Previously:

    // Shape
    DEFINE_PROPERTY_REF(PROP_SHAPE, Shape, shape, QString, "Sphere");

[HifiExperiments]

It looks like in EntityItemProperties.h LinePoints have moved from Polyline to Common section (l.235):

yes this was intentional! they are shared by Line and Polyline

In EntityItemProperties.h shape property definition has changed from QString to EntityShape

this was also intentional, it will take less space in a packet

[ksuprynowicz]

I noticed that ADD_GROUP_PROPERTY_TO_MAP_WITH_RANGE is no longer used. Earlier it was used in things like RingGizmoPropertyGroup, but in the PR it's replaced with ADD_GROUP_PROPERTY_TO_MAP.

[ksuprynowicz]

In EntityPropertyFlags.h PROP_AMBIENT_LIGHT_MODE moved from Ambient light to ambientOcclusion

[ksuprynowicz]

Some other properties moved to ambientOcclusion too:

	PROP_SKYBOX_MODE = PROP_DERIVED_48,
	PROP_HAZE_MODE = PROP_DERIVED_49,
	PROP_BLOOM_MODE = PROP_DERIVED_50,
	PROP_AVATAR_PRIORITY = PROP_DERIVED_51,
	PROP_SCREENSHARE = PROP_DERIVED_52,
	PROP_TONEMAPPING_MODE = PROP_DERIVED_53,

[ksuprynowicz]

In GrabPropertyGroup.h

COPY_GROUP_PROPERTY_TO_QSCRIPTVALUE_IF_URL_PERMISSION(PROP_GRAB_EQUIPPABLE_INDICATOR_URL ...

changed to

COPY_GROUP_PROPERTY_TO_QSCRIPTVALUE(PROP_GRAB_EQUIPPABLE_INDICATOR_URL

[ksuprynowicz]

Is legacy property support being removed from KeyLightPropertyGroup::copyFromScriptValue?

    // legacy property support
    COPY_PROPERTY_FROM_QSCRIPTVALUE_GETTER(keyLightColor, u8vec3Color, setColor, getColor);
    COPY_PROPERTY_FROM_QSCRIPTVALUE_GETTER(keyLightIntensity, float, setIntensity, getIntensity);
    COPY_PROPERTY_FROM_QSCRIPTVALUE_GETTER(keyLightDirection, vec3, setDirection, getDirection);
    COPY_PROPERTY_FROM_QSCRIPTVALUE_GETTER(keyLightCastShadows, bool, setCastShadows, getCastShadows);

[ksuprynowicz]

Should PolyLineEntityItem::getEntityProperties be empty?

EntityPropertyFlags PolyLineEntityItem::getEntityProperties(EncodeBitstreamParams& params) const {
    EntityPropertyFlags requestedProperties = EntityItem::getEntityProperties(params);

    return requestedProperties;
}

Previously:

EntityPropertyFlags PolyLineEntityItem::getEntityProperties(EncodeBitstreamParams& params) const {
    EntityPropertyFlags requestedProperties = EntityItem::getEntityProperties(params);
    requestedProperties += PROP_COLOR;
    requestedProperties += PROP_TEXTURES;

    requestedProperties += PROP_LINE_POINTS;
    requestedProperties += PROP_STROKE_WIDTHS;
    requestedProperties += PROP_STROKE_NORMALS;
    requestedProperties += PROP_STROKE_COLORS;
    requestedProperties += PROP_IS_UV_MODE_STRETCH;
    requestedProperties += PROP_LINE_GLOW;
    requestedProperties += PROP_LINE_FACE_CAMERA;
    return requestedProperties;
}

[ksuprynowicz]

ShapeEntityItem::getProperties seems to be missing properties._shapeChanged = false;.

[HifiExperiments]

I noticed that ADD_GROUP_PROPERTY_TO_MAP_WITH_RANGE is no longer used. Earlier it was used in things like RingGizmoPropertyGroup, but in the PR it's replaced with ADD_GROUP_PROPERTY_TO_MAP.

ooo good catch, I had a copy/paste bug and min/max wasn't properly handling minus signs or 0s

In EntityPropertyFlags.h PROP_AMBIENT_LIGHT_MODE moved from Ambient light to ambientOcclusion Some other properties moved to ambientOcclusion too:

	PROP_SKYBOX_MODE = PROP_DERIVED_48,
	PROP_HAZE_MODE = PROP_DERIVED_49,
	PROP_BLOOM_MODE = PROP_DERIVED_50,
	PROP_AVATAR_PRIORITY = PROP_DERIVED_51,
	PROP_SCREENSHARE = PROP_DERIVED_52,
	PROP_TONEMAPPING_MODE = PROP_DERIVED_53,

ah so this is just a comment so it doesn't really matter, but the reason it's like that is because Zones randomly have the group properties first, whereas most other types have the group properties last. I could move the non-group properties up if we thought it was clearer, I was mostly just leaving it how it had been.

In GrabPropertyGroup.h

COPY_GROUP_PROPERTY_TO_QSCRIPTVALUE_IF_URL_PERMISSION(PROP_GRAB_EQUIPPABLE_INDICATOR_URL ...

changed to

COPY_GROUP_PROPERTY_TO_QSCRIPTVALUE(PROP_GRAB_EQUIPPABLE_INDICATOR_URL

🙏 fixed

Is legacy property support being removed from KeyLightPropertyGroup::copyFromScriptValue?

whoops, that wasn't intentional, I've put it back!

Should PolyLineEntityItem::getEntityProperties be empty?

oof definitely not, I accidentally deleted an S from the end of the replacement variable name, great catch!

ShapeEntityItem::getProperties seems to be missing properties._shapeChanged = false;.

I always wondered why that was there, COPY_ENTITY_PROPERTY_TO_PROPERTIES already sets this to false so it was unnecessary.

[ksuprynowicz]

I think it's ready to merge :)