Skip to content

Commit

Permalink
Trade: use LayerName instead of $LayerName in MaterialData.
Browse files Browse the repository at this point in the history
Reason is that Assimp custom material attribute names are also prefixed
with $ and other weird characters, which could lead to them appearing
before $LayerName, causing a layer to falsely appear unnamed. A space,
instead, is before all printable characters so it's guaranteed to be
always first.

Some things you just don't realize at first. Fortunately the binary
layout isn't pinned yet for the serialization format so this change is
mostly fine.
  • Loading branch information
mosra committed Nov 27, 2021
1 parent 491751b commit aeeff73
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 59 deletions.
2 changes: 1 addition & 1 deletion src/Magnum/MeshTools/sceneconverter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ used.)")
/* Ignore layer name (which is always first) unless it's in
the base material, in which case we print it as it
wouldn't otherwise be shown anywhere */
if(i && !j && info.data.attributeName(i, j) == "$LayerName")
if(i && !j && info.data.attributeName(i, j) == " LayerName")
continue;

d << Debug::newline << indent
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

/* See Magnum/Trade/MaterialData.cpp and Magnum/Trade/Test/MaterialDataTest.cpp */
#ifdef _c
_cnt(LayerName,"$LayerName",String,Containers::StringView)
_cnt(LayerName," LayerName",String,Containers::StringView)
_c(AlphaMask,Float)
_ct(AlphaBlend,Bool,bool)
_ct(DoubleSided,Bool,bool)
Expand Down
8 changes: 4 additions & 4 deletions src/Magnum/Trade/MaterialData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ Containers::StringView MaterialData::attributeString(const MaterialAttribute nam
UnsignedInt MaterialData::layerFor(const Containers::StringView layer) const {
for(std::size_t i = 1; i < _layerOffsets.size(); ++i) {
if(_layerOffsets[i] > _layerOffsets[i - 1] &&
_data[_layerOffsets[i - 1]].name() == "$LayerName"_s &&
_data[_layerOffsets[i - 1]].name() == " LayerName"_s &&
_data[_layerOffsets[i - 1]].value<Containers::StringView>() == layer)
return i;
}
Expand Down Expand Up @@ -348,7 +348,7 @@ Containers::StringView MaterialData::layerName(const UnsignedInt layer) const {
CORRADE_ASSERT(layer < layerCount(),
"Trade::MaterialData::layerName(): index" << layer << "out of range for" << layerCount() << "layers", {});
/* Deliberately ignore this attribute in the base material */
if(layer && _layerOffsets[layer] > _layerOffsets[layer - 1] && _data[_layerOffsets[layer - 1]].name() == "$LayerName")
if(layer && _layerOffsets[layer] > _layerOffsets[layer - 1] && _data[_layerOffsets[layer - 1]].name() == " LayerName")
return _data[_layerOffsets[layer - 1]].value<Containers::StringView>();
return {};
}
Expand Down Expand Up @@ -953,9 +953,9 @@ Debug& operator<<(Debug& debug, const MaterialAttribute value) {
if(UnsignedInt(value) - 1 >= Containers::arraySize(AttributeMap))
return debug << "(" << Debug::nospace << reinterpret_cast<void*>(UnsignedInt(value)) << Debug::nospace << ")";

/* LayerName is prefixed with $, drop that */
/* LayerName is prefixed with a single space, drop that */
Containers::StringView string = AttributeMap[UnsignedInt(value) - 1].name;
if(string[0] == '$') string = string.suffix(1);
if(string[0] == ' ') string = string.suffix(1);

return debug << "::" << Debug::nospace << string;
}
Expand Down
17 changes: 9 additions & 8 deletions src/Magnum/Trade/MaterialData.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ MAGNUM_TRADE_EXPORT Debug& operator<<(Debug& debug, MaterialLayer value);
Convenience aliases to actual attribute name strings. In most cases the alias
is in the same form and capitalization --- so for example
@ref MaterialAttribute::DoubleSided is an alias for @cpp "DoubleSided" @ce, the
only exception is @ref MaterialAttribute::LayerName which is @cpp "$LayerName" @ce.
only exception is @ref MaterialAttribute::LayerName which is @cpp " LayerName" @ce (with a space at the front).
When this enum si used in
@ref MaterialAttributeData constructors, the data are additionally checked for
Expand All @@ -108,9 +108,9 @@ enum class MaterialAttribute: UnsignedInt {
* Layer name, @ref MaterialAttributeType::String.
*
* Unlike other attributes where string name matches the enum name, in this
* case the corresponding string is @cpp "$LayerName" @ce, done in order to
* have the layer name attribute appear first in each layer and thus
* simplify layer implementation.
* case the corresponding string is @cpp " LayerName" @ce (with a space at
* the front), done in order to have the layer name attribute appear first
* in each layer and thus simplify layer implementation.
* @see @ref MaterialData::layerName()
*/
LayerName = 1,
Expand Down Expand Up @@ -1591,10 +1591,11 @@ already sorted by name.
@subsection Trade-MaterialData-populating-custom Custom material attributes
While attribute names beginning with uppercase letters are reserved for builtin
Magnum attributes, anything beginning with a lowercase letter or a non-letter
can be a custom attribute. For greater flexibility, custom attributes can be
also strings or pointers, allowing you to store arbitrary properties or direct
While attribute names beginning with uppercase letters and whitespace are
reserved for builtin Magnum attributes, anything beginning with a lowercase
letter or a printable non-letter character can be a custom attribute. For
greater flexibility, custom attributes can be also strings or pointers,
allowing you to store arbitrary properties such as image filenames or direct
texture pointers instead of IDs:
@snippet MagnumTrade.cpp MaterialData-populating-custom
Expand Down
Loading

0 comments on commit aeeff73

Please sign in to comment.