SketchUp / api-issue-tracker

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

C API: can't cast Image to ComponentInstance #662

Open vanjac opened 3 years ago

vanjac commented 3 years ago

A SUImageRef should be a subtype of SUComponentInstance, just like a SUGroupRef is a subtype of SUComponentInstance (ie. there should be a SUImageToComponentInstance function). It's already possible to obtain ComponentInstanceRefs from ImageRefs if you get the image definition and then go through its instances. They have matching IDs so I know they refer to the same object. Right now I'm storing these in a hashmap so I can obtain the ComponentInstance when I need it, but it would be nice if this was part of the API.

vanjac commented 3 years ago

Looks like there was some discussion about this already in #303. And I saw that SUImageGetDefinition was added in 2021.1, which helps a bit

DanRathbun commented 3 years ago

@vanjac : It's already possible to obtain ComponentInstanceRefs from ImageRefs if you get the image definition and then go through its instances. They have matching IDs so I know they refer to the same object. Right now I'm storing these in a hashmap so I can obtain the ComponentInstance when I need it, but it would be nice if this was part of the API.

Why would you or anyone else need to cast to a SUComponentInstanceRef ?

Ie, what is the SUImageRef lacking that should be implemented rather than expose it to functions that do not apply to it, and quite likely would result in model corruption ? (Keep in mind that the interfaces for image objects are limited and differ from normal component instances by design.)

I might guess the attachment (gluing) related functions.

The dynamic component and classification attribute dictionaries can already be accessed via SUEntityRef functions. Nothing else that the SUComponentInstanceRef has applies to SUImageRef objects.

vanjac commented 3 years ago

It's just to reduce code duplication. I have a common method to handle any type of instance (group, component, image). It gets the definition, name, transform, etc. to build a scene graph. I don't want to copy all of that for Images when they have basically the same behavior.

DanRathbun commented 3 years ago

As said in #303, an ImageRef does not have a name property. (This was always a no-op and has been deprecated this past release.)

... when they have basically the same behavior.

The point is that they don't. They have a subset of the behavior and always had limited access via the APIs because messing with them could easily corrupt the file which would often crash SketchUp. I hope this is lessened with the new file format (however this will not help 2017 Make users.)

It's just to reduce code duplication. I have a common method to handle any type of instance (group, component, image).

I cannot think Trimble would undo their careful separation of images from the other instance types. (We'll see what Thomas thinks.)

ImageRef now have their own definition getter and always had their own transform getter. At some place your code determines that it is dealing with an image object. Would there be a way to conditionally use other functions, perhaps using function pointers, so that 1 method could handle both images and the other instance types ?

vanjac commented 3 years ago

My current roundabout method of getting the ComponentInstanceRef works well enough for me. Function pointers would just be overkill. I was hoping this could be exposed more easily in the API since it clearly exists beneath the surface.

Looking through the ComponentInstanceRef docs... which of these functions exactly would cause corruption when used on images? Only difference I see is the GUID, gluing, and locked state.

DanRathbun commented 3 years ago

The solid boolean methods (in the Ruby API) are also a difference. (They've not yet been added to the C API.)

DanRathbun commented 3 years ago

Looking through the SUComponentInstanceRef docs... which of these functions exactly would cause corruption when used on images?

Thinking back, I believe it was mostly misusing an image's material (and it's texture, which is actually accessed via SUDrawingElementRef,) and manipulating it's geometry (which is accessed via it's definition's entities.)

So I'll soften this ... "rather than expose it to functions that do not apply to it, and quite likely would result in model corruption ? " ... to acknowledge that file/model dB corruption might not be of the highest concern (especially with the new file format.)

However, encouraging the cast might require the SUComponentInstanceRef API functions that do not apply to images, to have additional error handling added to return specific results (usually SU_ERROR_NO_DATA) when called on image objects.

thomthom commented 3 years ago

Looking through the ComponentInstanceRef docs... which of these functions exactly would cause corruption when used on images? Only difference I see is the GUID, gluing, and locked state.

Altering the face and four edges along with the material would break SketchUp's hard coded expectations of an Image.

The fact that Image is a sub-class of ComponentInstance under the hood was a bad choice because an Image is not an invariant of ComponentInstance. Though that was done many many years ago and we're stuck with it. Since the Ruby API did expose the Image definitions we did eventually allow the C API to access it, for the sake of parsing (reading) convenience as you describe.

DanRathbun commented 3 years ago

@thomthom : The fact that Image is a sub-class of ComponentInstance under the hood was a bad choice because an Image is not an invariant of ComponentInstance.

Would you agree with me when I said ...

@DanRathbun : _However, encouraging the cast might require the SUComponentInstanceRef API functions that do not apply to images, to have additional error handling added to return specific results (usually SU_ERROR_NODATA) when called on image objects.

And ...

@DanRathbun : I cannot think Trimble would undo their careful separation of images from the other instance types. (We'll see what Thomas thinks.)

Ie, I'm interested in if you think that adding a casting function is a good idea given the use case is to reduce a single instance of code duplication ?

Regardless, such a casting function would be a good place to put a warning into the documentation not to use certain functions with image objects.


I am still wondering if there is a way to type check objects at runtime in C so, for example, to get a name property. Ie, an image instance does not have a name property, but a component (and a group) does. So there would need to be a conditional statement to get the image definition's name in the case of an image.

Reading up on the C typeof operator didn't help me much.

thomthom commented 3 years ago

Ie, I'm interested in if you think that adding a casting function is a good idea given the use case is to reduce a single instance of code duplication ?

It wouldn't be a highest priority. Particularly since there is a certain amount of risk of being misused in ways that could lead to model corruption. I'm already uncomfortable with how image entities already expose their internal implementation details.