[al] Add support for default rgba values and color threshold for exporting #2250
Conversation
| MString& code, | ||
| float value, | ||
| int precision, | ||
| const std::string& controller, |
There was a problem hiding this comment.
I would be nicer to pass as MString, other parameters are passed using that type (except prefix for some reason). It gets converted to a MString in the function anwyay. Other wise the parameters would now take all three of: const char*, MString and std::string.
| float value, | ||
| int precision, | ||
| const char* controller, | ||
| bool state) |
There was a problem hiding this comment.
Below, in generateFloatGlobals, you receive this as enableState, which I think is a better name that "state". It makes the purpose of the parameter clearer. I would change it here to be the same.
| const float& defVal, | ||
| int defPre, | ||
| const char* defController, | ||
| bool defState) |
There was a problem hiding this comment.
Same here, I'd rename defState to enableState.
| @@ -387,6 +410,9 @@ class PluginTranslatorOptions | |||
| MString defString; ///< default string value | |||
| std::vector<MString> enumStrings; ///< the text values for the enums | |||
| OptionType type; ///< the type of the option | |||
| int precision { 1 }; | |||
| std::string controller {}; | |||
There was a problem hiding this comment.
Same here: I'd use a MString instead of std::string.
| } | ||
| } | ||
|
|
||
| /// \brief Test exporting mesh with color threshold value and compaction 1 (basic level) |
There was a problem hiding this comment.
Comment does not match: claims test uses compaction 1 (basic level), but the test uses 3.
| static constexpr const char* const kMeshDefaultColourRGB = "Default RGB"; ///< default rgb values | ||
| static constexpr const char* const kMeshDefaultColourA = "Default Alpha"; ///< default alpha values | ||
| static constexpr const char* const kCustomColourThreshold | ||
| = "Custom Colour Threshold"; ///< default alpha values |
There was a problem hiding this comment.
Comment here and on the next does not corresponds.
|
|
||
| // check for per-vertex assignment | ||
| std::vector<uint32_t> indicesMap; | ||
| indicesMap.resize(numPoints, -1); |
There was a problem hiding this comment.
You are using -1 here but 0xFFFFFFFF below. I would be clearfer if they were both the same. (Also, passing a negative value to initialize an unsigned integer without a cast might trigger a warning on some compilers and we generally have the warning-as-error compiler option turned on.)
There was a problem hiding this comment.
I have updated the code to check against the visited index, it is less memory friendly but more robust, let me know if that looks better?
There was a problem hiding this comment.
This is in the AL plugin, if you feel the change would impact performance, you can revert it. The callers are from data you control, if you feel confident the index are always valid by construction, feel free to remove the check, but I'd advise to do profiling first to verify if it really is a bottleneck.
There was a problem hiding this comment.
I reverted the changes in DiffPrimVar.cpp.
I have to admit that it was a copy-n-paste from guessColourSetInterpolationTypeExtensive() and the code has been used in production for a long time, I would keep it untouched in that case.
| indicesMap.resize(numPoints, -1); | ||
| for (uint32_t pntInx = 0, n = pointIndices.length(); pntInx < n; ++pntInx) { | ||
| auto index = pointIndices[pntInx]; | ||
| auto lastIndex = indicesMap[index]; |
There was a problem hiding this comment.
For robustness reason, I would suggest verifying that the index is within the indicesMap size, in case of data corruption. Just resize the indicesMap to the new-found size.
seando-adsk
added
the
ready-for-merge
This PR mainly contains two features for exporting: