ENUM metadata support, treating them as scalars rather than using a container type

[azrogers]

This is an alternative version of #1085, implementing enums by leveraging our existing scalar metadata support rather than implementing them as an entirely new container type. This requires some changes to the assumptions we're currently making in the metadata system, chief among them the breaking of the connection between C++ type and PropertyType. However, it does simplify the implementation and reduce the amount of copy-and-paste necessary to support enums. Please review and discuss which approach you prefer!

[kring]

I have a few comments below, but overall this looks reasonable to me. I'm going to look at the Unreal implementation next, though, so that could change my opinion!

[j9liu]

@azrogers I haven't done a full review, but I at least wanted to check the areas that Kevin commented on. I'll do another pass early next week

[j9liu]

@azrogers Thank you for responding quickly to feedback and putting effort into these changes!

My one remaining reservation is about the way that we handle the constructors with pEnumDefinition. There is a silent assumption that if we call the constructor that includes the enum definition parameter -- even if nullptr -- that the property must have been an enum. I feel like this isn't right; we could be handling the inputs more generally, starting with PropertyView then working its way up into PropertyTablePropertyView, etc.

Let me know what you think -- overall I think it's going to reduce duplication.

[j9liu]

Might be nitpicky, but can be flatten the if else here so there's less indentation? It helps with readability at the end of the day

[azrogers]

I don't see how it can be flattened?

[j9liu]

Sorry, I just meant the outer if/else statement. Like this:

Suggested change

	if (enumDefinitionIt == this->_pEnumDefinitions->end()) {
	callback(
	propertyId,
	PropertyTablePropertyView<uint8_t>(
	PropertyTablePropertyViewStatus::ErrorInvalidEnum));
	return;
	} else {
	componentType = convertStringToPropertyComponentType(
	enumDefinitionIt->second.valueType);
	if (componentType == PropertyComponentType::Float32 ||
	componentType == PropertyComponentType::Float64) {
	callback(
	propertyId,
	PropertyTablePropertyView<uint8_t>(
	PropertyTablePropertyViewStatus::ErrorInvalidEnum));
	return;
	}
	}
	if (enumDefinitionIt == this->_pEnumDefinitions->end()) {
	callback(
	propertyId,
	PropertyTablePropertyView<uint8_t>(
	PropertyTablePropertyViewStatus::ErrorInvalidEnum));
	return;
	}
	componentType = convertStringToPropertyComponentType(
	enumDefinitionIt->second.valueType);
	if (componentType == PropertyComponentType::Float32 ||
	componentType == PropertyComponentType::Float64) {
	callback(
	propertyId,
	PropertyTablePropertyView<uint8_t>(
	PropertyTablePropertyViewStatus::ErrorInvalidEnum));
	return;
	}

[j9liu]

Same with all the others.

[j9liu]

Could we flatten this if/else?

[azrogers]

Similarly, I don't see how this can be flattened. We can't do the lookup unless it's an enum value.

[j9liu]

Suggested change

	if (enumDefinitionIt == this->_pEnumDefinitions->end()) {
	return PropertyTexturePropertyView<T, Normalized>(
	PropertyTexturePropertyViewStatus::ErrorInvalidEnum);
	} else {
	componentType = convertStringToPropertyComponentType(
	enumDefinitionIt->second.valueType);
	pEnumDefinition = &enumDefinitionIt->second;
	if (componentType == PropertyComponentType::Float32 ||
	componentType == PropertyComponentType::Float64) {
	return PropertyTexturePropertyView<T, Normalized>(
	PropertyTexturePropertyViewStatus::ErrorInvalidEnum);
	}
	}
	if (enumDefinitionIt == this->_pEnumDefinitions->end()) {
	return PropertyTexturePropertyView<T, Normalized>(
	PropertyTexturePropertyViewStatus::ErrorInvalidEnum);
	}
	componentType = convertStringToPropertyComponentType(
	enumDefinitionIt->second.valueType);
	pEnumDefinition = &enumDefinitionIt->second;
	if (componentType == PropertyComponentType::Float32 ||
	componentType == PropertyComponentType::Float64) {
	return PropertyTexturePropertyView<T, Normalized>(
	PropertyTexturePropertyViewStatus::ErrorInvalidEnum);
	}

[j9liu]

Could we flatten this if/else?

[azrogers]

Same here, how would you flatten it?

[j9liu]

Suggested change

	if (enumDefinitionIt == this->_pEnumDefinitions->end()) {
	return PropertyTexturePropertyView<PropertyArrayView<T>, Normalized>(
	PropertyTexturePropertyViewStatus::ErrorInvalidEnum);
	} else {
	componentType = convertStringToPropertyComponentType(
	enumDefinitionIt->second.valueType);
	pEnumDefinition = &enumDefinitionIt->second;
	if (componentType == PropertyComponentType::Float32 ||
	componentType == PropertyComponentType::Float64) {
	return PropertyTexturePropertyView<PropertyArrayView<T>, Normalized>(
	PropertyTexturePropertyViewStatus::ErrorInvalidEnum);
	}
	}
	if (enumDefinitionIt == this->_pEnumDefinitions->end()) {
	return PropertyTexturePropertyView<PropertyArrayView<T>, Normalized>(
	PropertyTexturePropertyViewStatus::ErrorInvalidEnum);
	}
	componentType = convertStringToPropertyComponentType(
	enumDefinitionIt->second.valueType);
	pEnumDefinition = &enumDefinitionIt->second;
	if (componentType == PropertyComponentType::Float32 ||
	componentType == PropertyComponentType::Float64) {
	return PropertyTexturePropertyView<PropertyArrayView<T>, Normalized>(
	PropertyTexturePropertyViewStatus::ErrorInvalidEnum);
	}

[azrogers]

@j9liu Addressed your review comments. Was able to consolidate most of the constructors - I did have to add a pEnumDefinition parameter to some of the protected constructors that would never be enums, just so the templating worked out, but since the parameter is ignored I don't think it's a big deal.