tommykaneko / sketchup-api-c-wrapper Goto Github PK
View Code? Open in Web Editor NEWA complete set of C++ Wrapper classes for SketchUp C API objects
License: MIT License
A complete set of C++ Wrapper classes for SketchUp C API objects
License: MIT License
a char* char_array
needs to be null terminated. A vector<char>
does not. No need to add 1 here.
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).
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;
Sketchup-API-C-Wrapper/SUAPI-CppWrapper/model/TypedValue.cpp
Lines 418 to 426 in 8fb0695
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;
}
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;
}
create_typed_value
like the other getters?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;
}
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
My initial CMake project implementation in #42 only added Visual Studio support. This needs to be expanded with XCode support.
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.
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.
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;
}
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 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.
I'm getting errors with SU_RESULT
here, and wherever else it's used. Should it not be SUResult
?
I was setting up tests for the current behaviour or Radians:
Sketchup-API-C-Wrapper/src/SUAPI-CppWrapper/Geometry.cpp
Lines 47 to 58 in 94aa35c
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?
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?
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.
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:
Sketchup-API-C-Wrapper/SUAPI-CppWrapper/Transformation.cpp
Lines 75 to 80 in 498c19b
That's better replaced with SUTransformationTranslation
Sketchup-API-C-Wrapper/SUAPI-CppWrapper/Transformation.cpp
Lines 29 to 33 in 498c19b
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?
Need to add SUStringCreate(&name_string);
after this declaration.
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'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:
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 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.
Sketchup-API-C-Wrapper/SUAPI-CppWrapper/Transformation.hpp
Lines 48 to 53 in 498c19b
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:
Sketchup-API-C-Wrapper/SUAPI-CppWrapper/Transformation.hpp
Lines 74 to 79 in 498c19b
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'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.
I have started this and will have a pull request for ShadowInfo this evening.
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
I think each element of the array may need one or both of SUSetInvalid()
and SUTypedValueCreate()
before using the array.
Sketchup-API-C-Wrapper/SUAPI-CppWrapper/Transformation.cpp
Lines 68 to 73 in 498c19b
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.
Not sure what this project's C++ version policy is, but since C++17 we have [[maybe_unused]]
that can suppress unused variable warnings.
Sketchup-API-C-Wrapper/src/SUAPI-CppWrapper/model/TypedValue.cpp
Lines 28 to 29 in 6879346
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'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 noticed that the getters for TypedValue doesn't check or handle asking for the wrong type. Would it make sense to raise an error?
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.
So this vector goes away at the end of the function call, correct? Does each element need SUStringRelease
?
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.