SketchUp / api-issue-tracker

Public issue tracker for the SketchUp and LayOut's APIs
https://developer.sketchup.com/
39 stars 10 forks source link

SUEntityGetAttributeDictionaries can return unexpected number of objects #584

Open thomthom opened 3 years ago

thomthom commented 3 years ago

SUEntityGetAttributeDictionaries can return fewer items than what SUEntityGetNumAttributeDictionaries indicates.

One identified scenario is when a face has a positioned texture.

2020-12-08_15h50_40

The screenshot above has three faces, from left to right:

faces.zip

SketchUp has internally a distinction between "attributes" and "named attributes". That attributes exposed to the APIs are "named attributes". However SUEntityGetNumAttributeDictionaries returns a count of all the attributes instead of filtering by what SUEntityGetAttributeDictionaries will actually return.

SUEntityGetNumAttributeDictionaries should return a count that is relevant for SUEntityGetAttributeDictionaries.

There's a risk of the API user crashing if they don't validate the count param and that the entities in out array is valid before using it.

DanRathbun commented 3 years ago

So referring to the docs for the two functions ... should a coder use the same integer variable for len and count ?

thomthom commented 3 years ago

You could - it depends on what you do afterwards.

Here's the code that flushed out this bug, revised to take into account this behaviour;

SUAttributeDictionaryRef GetDictionary(const SUEntityRef& entity_ref,
  const std::string& name)
{
  SUAttributeDictionaryRef dictionary_ref = SU_INVALID;

  size_t num_dictionaries = 0;
  SU(SUEntityGetNumAttributeDictionaries(entity_ref, &num_dictionaries));
  if (num_dictionaries == 0) {
    return dictionary_ref;
  }

  std::vector<SUAttributeDictionaryRef> dictionary_refs(num_dictionaries, SU_INVALID);
  SU(SUEntityGetAttributeDictionaries(entity_ref, dictionary_refs.size(),
      dictionary_refs.data(), &num_dictionaries));

  auto it = std::find_if(std::begin(dictionary_refs), std::end(dictionary_refs),
    [&name](const auto& dictionary) {
      // A bug in the SketchUp API will cause
      // SUEntityGetNumAttributeDictionaries to yield a count higher than the
      // number of dictionaries will actually be returned.
      // This can for instance be if a face has a positioned texture, as the
      // texture data is stored in a private dictionary that the APIs cannot
      // access.
      // This means we need to account for the possibility that some of the
      // items in the vector isn't valid.
      return SUIsValid(dictionary) && GetName(dictionary) == name;
    });
  if (it != std::end(dictionary_refs)) {
    dictionary_ref = *it;
  }

  return dictionary_ref;
}

I'm not using num_dictionaries. I could maybe have resized the vector back down to num_dictionaries afterwards - but that would be assuming that the returned results where continuous. They are in this case - but this is the paranoid safe way handle it.

sketchupbot commented 3 years ago

Logged as: SKEXT-2846