tommykaneko / sketchup-api-c-wrapper Goto Github PK

I have started this and will have a pull request for ShadowInfo this evening.

Not sure what this project's C++ version policy is, but since C++17 we have [[maybe_unused]] that can suppress unused variable warnings.

I'm getting errors with SU_RESULT here, and wherever else it's used. Should it not be SUResult?

My initial CMake project implementation in #42 only added Visual Studio support. This needs to be expanded with XCode support.

https://github.com/TommyKaneko/Sketchup-API-C-Wrapper/blob/master/SUAPI-CppWrapper/model/Edge.hpp#L114

I'm not sure if having the ! operator implemented. I think code would read better if you have to call edge.IsValid(). Having that operator there almost makes it seem like a pointer.

I've been having trouble reading mixed-type TypedValue arrays from a model on disc. I wrote a test which I will reference here in a pull request.

A "mixed-type" array would be an array such as [1, 2.0, "three"] stored in an AttributeDictionary for example.

The issue I have is I get several different results from running the test. One of the following happens:

1. The test passes.
2. The test fails at the ASSERT with count being 0, 1 or 2
3. The test fails with unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.

Does my test code have a problem? If not then there's probably a bug in the wrapper lib or SketchUpAPI.

I would recommend raising the error level from /W3 to /W4 to catch more warnings/errors.

But raising the level will flush out new errors in the project, so they needs to be addressed at the same time.

https://github.com/thomthom/Sketchup-API-C-Wrapper/blob/eba8b51aa1037ab8109255fe97cf20f943960bdb/cmake/SketchUpAPICpp.cmake#L12-L14

This cause a copy of the passed in array. Better to take the param as a const ref:

std::array<double, 4> multiply4x1(const std::array<double, 4>& matrix4_1) const;

Similarly:

Better off avoiding copying the parmeters:

  Transformation(const SUTransformation& transformation);
  Transformation(const Axes& axes, const Vector3D& translation, double scalar);
  Transformation(const Vector3D& x_axis, const Vector3D& y_axis, const Vector3D& z_axis, const Vector3D& translation, double scalar);  
  explicit Transformation(double scalar);
  explicit Transformation(const Vector3D& translation);
  explicit Transformation(const Point3D& translation);

For the *Ref types from the C API its ok to pass by value, they are basically just a pointer - light-weight.

I only peeked at Transformation.hpp - but it's be good to address this throughout the project.

I think each element of the array may need one or both of SUSetInvalid() and SUTypedValueCreate() before using the array.

So this vector goes away at the end of the function call, correct? Does each element need SUStringRelease?

I know this isn't really an issue but I don't know where else to ask this, but can someone provide me with a function that returns all the vertices of the entire file in a list with their correct transforms? I'm struggling to figure out the sketchup file structure and am always missing some faces/vertices.

This is producing errors - and will not work. Either it should return a const ref of the internal member, or a Ref copy.

SUFaceRef Face::ref() const {  return SUFaceFromEntity(m_entity); }


Face::operator SUFaceRef() const {  return this->ref();}


Face::operator SUFaceRef*() {
  // TODO: test if the solution below works.
  SUFaceRef face = this->ref();
  return &face;
}

http://editorconfig.org/

Many editors and IDE's already support it, and those that do not likely have a plugin. It can help maintain a consistent style.

You're on a a Mac I think, but Visual Studio has built-in support.

The editor in Visual Studio supports the core set of EditorConfig properties:

indent_style
indent_size
tab_width
end_of_line
charset
trim_trailing_whitespace
insert_final_newline
root

Should AttributeDictionary inherit from Entity?

If I remember correctly, Classifications are implemented using a hierarchy of Dictionaries, and so to access all that you need to get the attribute_dictionaries() of an AttributeDictionary through the parent Entity class.

I was setting up tests for the current behaviour or Radians:

From what I understand of the code it's trying to cap the value by rolling around 2*PI.

For 7.0 I get 0.71681469282041377 which I'd expect.
For -7.0 I get 5.5663706143591725 which I did not expect.

I would have thought I should get -0.71681469282041377.

Am I wrong? Can you explain the intent so I can fill in test unit for this?

Avoid creating a scaling transformation like this. Instead scale the items in the 3x3 part of the matrix.
The SU API used to do this - but it caused problems within some parts of SketchUp and several extensions.

Hey, thanks for putting this up.
I am going to try to make a golang wrapper for this.

If you have anything to say about it let me know..

I have made a separate branch, with the idea of merging into the master after consultation.
https://github.com/TommyKaneko/Sketchup-API-C-Wrapper/tree/single_member

On this branch: Removed the member variables of classes derived from Entity. Only the m_entity member variable is needed in this derived classes. To get the derived class member, the m_entity member is downcast to get the required class member.

Please test for your own purposes and let me know if you agree with merging it into the master branch.

I think this is the way to go, as it removes member variables which reduce the size of each object. Also may be easier to maintain a class with just one member as opposed to 3 in most cases (eg Entity, DrawingElement, Edge).

Couple of notes/questions related to this:

bool TypedValue::bool_value() const {
  if (!(*this)) {
    return 0;
  }
  bool bool_val;
  SUResult res = SUTypedValueGetBool(m_typed_value, &bool_val);
  assert(res == SU_ERROR_NONE);
  return bool_val;
}

1. It returns a numeric value when the return type is bool. (This can be caught by raising the warning level)
2. Why isn't this calling create_typed_value like the other getters?

Shouldn't the Axes class inherit from Entity rather than Drawingelement?

Update - apparently the C API has a method to upcast to both Entity and Drawingelement

Recently, I have been getting an error while using this library entirely due to my fault. I have been getting BAD_ACCESS errors, on the point when Model is destroyed and released. It's a pretty bad error, as it's difficult to find out where the issue arose.

It turns out that I was directly adding a Material object from one model directly to another. So the model was releasing another model's material. Sketchup C API doesn't have a problem with doing this through SUModelAddMaterials. There are also other ways to get the same error by adding layers from another model.

@thomthom - do you have a view on whether the C Wrapper should be checking such errors made by the user? If we do check, it seems quite a performance hungry task. If we don't, these kinds of errors will be very difficult to diagnose for the user.

I'm not sure if the editorconfig file will be picked up with CMake out of source building.

Worth looking into how we can make sure this works. Maybe a step in the CMake config that copies it to the target build location.

Is there no projects set up for this wrapper?

I was going to look into setting up a PR for GoogleTest (ffc704c#r27718155)

For that to happen there needs to be either a static or dynamic lib for which the GoogleTest project can link with. And I had also assumed that's how it would have been consumed.,

How are you currently using it? Adding all the files into the same projects you are using it with?

Why this ifdef?

I usually only use <cmath>. Looking at <ctgmath> it appear to include <cmath> and `.

Why the need for different includes? Isn't <cmath> enough for this file?

I got the base for test units with GoogleTest working:

Current dev branch is here: https://github.com/thomthom/Sketchup-API-C-Wrapper/tree/dev-vs-project

Though it required me to move files around to allow for test project files. I'll write up a summary later on - then you can all evaluate it.

The wrapper classes uses inheritance, but their destructors are not marked as virtual:

If one of these derived classes were allocated on the heap (using new) then a delete on a base class pointer would lead to Undefined Behaviour.

These wrappers however are wrapping opaque pointers (refs) and probably passed around as values, in which case it wouldn't be a problem. However, the interface doesn't protect against incorrect usage.

If the objects are not meant to be deleted via a base pointer then their destructor should be non-virtual and protected.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-dtor

C.127: A class with a virtual function should have a virtual or protected destructor

Reason A class with a virtual function is usually (and in general) used via a pointer to base. Usually, the last user has to call delete on a pointer to base, often via a smart pointer to base, so the destructor should be public and virtual. Less commonly, if deletion through a pointer to base is not intended to be supported, the destructor should be protected and non-virtual; see C.35.

(Emphasis on the last paragraph is mine)

C.35: A base class destructor should be either public and virtual, or protected and non-virtual

Reason To prevent undefined behavior. If the destructor is public, then calling code can attempt to destroy a derived class object through a base class pointer, and the result is undefined if the base class’s destructor is non-virtual. If the destructor is protected, then calling code cannot destroy through a base class pointer and the destructor does not need to be virtual; it does need to be protected, not private, so that derived destructors can invoke it. In general, the writer of a base class does not know the appropriate action to be done upon destruction.

The current version of Transformation is doing a lot of direct manipulation of the matrix. The SU2018 version of the SDK added a lot new functions to the geometry types. Best to migrate as much as possible to use the new functions.

For example:

That's better replaced with SUTransformationTranslation

http://extensions.sketchup.com/developer_center/sketchup_c_api/sketchup/geometry_2transformation_8h.html

The current implementation of TypedValue::typed_value_array() leaks memory.
You allocate memory with SUTypedValueRef* values = new SUTypedValueRef[count]; but are using delete values instead of delete[] values to free the memory.

Just a suggestion on general coding guidelines for allocating memory;

In general new and delete should be avoided. Especially with C++11 and newer there are much safer ways to manage memory.

For reference, the Cpp Core Guidelines expand on this:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-new

As a side note, also prefer range-for loops over index based for loops: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-for-range

Further more, SUTypedValueRef* values = new SUTypedValueRef[count]; never initialize the data.

Here's an alternate version:

std::vector<TypedValue> TypedValue::typed_value_array() const {
  if (!(*this)) {
    return std::vector<TypedValue>();
  }
  size_t count = 0;
  SUResult res = SUTypedValueGetNumArrayItems(m_typed_value, &count);
  assert(res == SU_ERROR_NONE);
  std::vector<SUTypedValueRef> values(count, SU_INVALID);
  res = SUTypedValueGetArrayItems(m_typed_value, values.size(), values.data(), &count);
  assert(res == SU_ERROR_NONE);
  std::vector<TypedValue> typed_vals;
  typed_vals.reserve(count);
  for (const auto& value : values) {
    typed_vals.push_back(TypedValue(value));
  }
  return typed_vals;
}

This is safe in terms of memory allocations and range iterations.

Personally I'd go further - according with P.3. in the Cpp Core Guidelines and use std::transform to express intent when converting SUTypedValueRef to TypedValue:

#include <algorithm>

std::vector<TypedValue> TypedValue::typed_value_array() const {
  if (!(*this)) {
    return std::vector<TypedValue>();
  }
  size_t count = 0;
  SUResult res = SUTypedValueGetNumArrayItems(m_typed_value, &count);
  assert(res == SU_ERROR_NONE);
  std::vector<SUTypedValueRef> values(count, SU_INVALID);
  res = SUTypedValueGetArrayItems(m_typed_value, values.size(), values.data(), &count);
  assert(res == SU_ERROR_NONE);
  std::vector<TypedValue> typed_vals(count);
  std::transform(begin(values), end(values), begin(typed_vals),
    [](const SUTypedValueRef& value) {
      return TypedValue(value);
    });
  return typed_vals;
}

a char* char_array needs to be null terminated. A vector<char> does not. No need to add 1 here.

In this function, you are telling the API that your char array is one larger than it actually is. You already added one to account for the null terminator. I am getting a Heap Corruption exception because calling delete [] char_array tries to delete one too many bytes.

The line should be just:

res = SUStringGetUTF8(m_string, out_length, &char_array[0], &out_length);

Oh my test code:

#include <iostream>
#include <string>
#include <vector>

#include <SUAPI-CppWrapper\model\Model.hpp>
#include <SUAPI-CppWrapper\model\AttributeDictionary.hpp>

int main(int argc, char ** argv) {

    std::string filepath{ argv[1] };

    auto model = CW::Model(filepath);
    //std::cout << "Model version: " << model.version() << "\n";

    std::vector<CW::AttributeDictionary> dicts = model.attribute_dictionaries();
    for (auto dict : dicts) {
        std::cout << dict.get_name() << "\n";
        for (auto key : dict.get_keys()) {
            //std::cout << key << ": " << dict.get_value(key) << "\n";
        }
    }

    return 0;
}

Need to add SUStringCreate(&name_string); after this declaration.

Visual Studio have a feature that allow you to define how custom objects are represented in the debugger: https://msdn.microsoft.com/en-us/library/jj620914.aspx?f=255&MSPPError=-2147217396

Could be useful to allow the debugger to how what text a String object represent, or a Color object.

I noticed that the getters for TypedValue doesn't check or handle asking for the wrong type. Would it make sense to raise an error?

For me, it wasn't enough to initialize the array values to SU_INVALID. I also had to call SUStringCreate() on each element.

for (size_t i=0; i < num_keys; i++) {
  	keys_ref[i] = SU_INVALID;
        SUStringCreate(&keys_ref[i]);
}

I'm working with version 18.0.16975 SDK.