Fix DefinitionList.load - problems in some situations

[TommyK]

I have been having issues with DefinitionsList.load. I am trying to reload an external component that has been updated externally via Ruby.

I encounter the following behaviours:

model = Sketchup.active_model
# We have a single component in the model
old_definition = model.entities[0].definition
puts old_definition.path #=> '/HD/Users/User/cylinder.skp'
puts model.definitions.count #=> 1

# Note that the definition was saved to '/HD/Users/User/cylinder.skp', and 
# the file was then edited independently

# Attempt to reload the component
new_definition = model.definitions.load '/HD/Users/User/cylinder.skp'

puts old_definition.guid #=> 1736b32a-8dd3-3040-bdfd-62a05e188985
puts new_definition.guid #=> 1736b32a-8dd3-3040-bdfd-62a05e188985
# The same GUID! The file was NOT loaded, as it has been edited since.  It effectively loaded the internal component

# And a new (renamed) definition was not added to the definitions list:
puts model.definitions.count #=> 1
if new_definition == old_definition
puts "The definitions are the same! This shouldn't happen, no?"
end

# However, loading a new component that does not share a path with another component works:
second_new_definition = model.definitions.load '/HD/Users/User/cube.skp'
puts model.definitions.count #=>2

# Other things I have tried:
# - Renaming the old definition before load - same result:
old_definition.name= "new name"
new_definition = model.definitions.load '/HD/Users/User/cylinder.skp'
puts new_definition.name #=> "new_name"

It seems that Sketchup refuses to reload a component from a path if there is a component with that path in the model already. I can’t see this as intended behaviour, as it makes it difficult to update components that were edited elsewhere using Ruby.

[tt_su]

Hmm… I think I ran into this recently and have it on my list of things to look into. I suspect there might be a bug where SU mistakes the components to be identical.
I think I worked around it by modifying the definition I wanted to replace first - forcing it to be marked as changed. I either cleared the entities or added a cpoint - then proceeded to load the new comp.

[TommyK]

Thanks ThomThom! That totally worked!

[DanRathbun]

@tt_su: Has there been an API request for a Sketchup::ComponentDefinition#update() method ?

[TommyK]

Not from me, certainly. And having googled for my problem and finding little, i am inclined to think not many people are working on that kind of plugin.

That said, I would love to have it! To emulate the update() method you suggest, I need to put in 20 lines of performance expensive code, to workaround bugs like this and to replace the instances of the old component with the new.

[tt_su]

@DanRathbun

@tt_su: Has there been an API request for a Sketchup::ComponentDefinition#update() method ?

No that I know of. What would it do? Update the GUID?
If it’s to address the loading bug then it would be more appropriate to fix that instead of adding a new method to workaround. (Though I suspect that wasn’t your intention either…)

[DanRathbun]

No it would specifically update the definition from it’s path (local or remote url.)

I was going upon the assumption that, when loading a new definition with the DefinitionList methods, if a definition exists in the list already with the same name, then a NEW definition is created with a “differenced” name. (Appending “#2” at the end, etc.)

It IS conceivable that some people may wish to do this in order to change the second definition in some way, so as it will have it’s own different (but similar) instances in the model.

I seem to remember ACAD having a xref update command that would refresh the blocks in the DWG file. Perhaps it was also a similar command to update blocks loaded from a local block library.

Anyway… I was thinking similar to this type of command(s).

However, it makes more sense that the “update” method be called upon the actual definition instance, not the collection. (The method could be in both places, one calling the other. The one defined for the collection would take the instance to be updated, as the argument. Coders could use either as desired.)

So, my position might be that the current 2 load methods, are properly attached as instance methods of the collection, and both need a path argument because they have not yet been loaded into the definition list collection. Also that they mostly already do what they were meant to do. (Ie, the updating wasn’t foreseen as most plugin coders have implemented it.)

So,… I’d think that it would be better for those who have had to write their own method, to be able to simply replace their method call, with a call to a new update method.
It also makes more sense that after a definition is loaded, that an update call would be made by calling one of it’s instance methods.

But hey, you could fix the load method for the collection, and add an alias in the definition like so:

def update(from = self.path)
  self.model.definitions.load(from)
end

I guess my main point (besides receiver of the method,) is that is seem clunky to be forced to specify the path argument, when the instance already knows it.

Anyway, … just thinking out loud…

[TommyK]

I would +1 this proposal.

The main selling point for me is that even if the DefinitionList::load() method is fixed, to achieve what ComponentDefinition::update() would achieve, one would still have to load a component, replace all the instances of the old component with the new, delete the old definition by emptying its geometry (it’s the only way to delete a component from the definition list without purging), rename the new component back to the old name (because it’s name got changed automatically). Aside from the extra code required, it probably adds extra CPU load (this would need to be tested).

The only change I would suggest is to put in an optional parameter with the path to load. This is necessary for when working on a file between several computers, where the paths might not match.

So: ComponentDefinition::update(path)

Yes please!

[DanRathbun]

@TommyK

replace all the instances of the old component with the new

OH yea I forgot to stress this. The update after reloading the definition from storage, should refresh any instances that used the old definition, invalidate the view, etc.

---

Re the path arg, I showed an example in Ruby of such an optional arg that defaulted to the definition’s current path.

[tt_su]

@TommyK

one would still have to load a component, replace all the instances of the old component with the new, delete the old definition by emptying its geometry (it’s the only way to delete a component from the definition list without purging), rename the new component back to the old name (because it’s name got changed automatically). Aside from the extra code required, it probably adds extra CPU load (this would need to be tested).

Yes - there should be a way to force. In any case - the current behaviour sounds like a bug to me - where SU incorrectly thinks the definition haven’t changed. That I think we have an issue filed for already. I’ll double check. (SU-30933)

I’ll also file a request for a ComponentDefintion.reload - this makes sense to have. (SU-31274)

[tt_su]

@TommyK

This is necessary for when working on a file between several computers, where the paths might not match.

On that topic - I think SketchUp could do a better job at keeping references - looking for relative paths.

[DanRathbun]

@tt_su

I’ll also file a request for a ComponentDefinition.reload - this makes sense to have. (SU-31274)

Thanks… the naming should be API consistent, be it “update”, “reload” or “refresh”.

I used “update” because the Hash class uses it, (which also has the alias “merge!”.)

This brings up the issue of whether it should be a ‘banged’ (dangerous) method name ?

[tt_su]

@DanRathbun

Thanks… the naming should be API consistent, be it “update”, “reload” or “refresh”.

I used “update” because the Hash class uses it, (which also has the alias “merge!”.)

Yes - implementation details such as names will be reviewed upon implementation. Compared against the rest of our API and Ruby itself. Historically the API hasn’t always been consistent - which is annoying.

@DanRathbun

This brings up the issue of whether it should be a ‘banged’ (dangerous) method name ?

It should not be banged. Bangs are supposed to be used for a mutator only if there is a non-mutation version. Example is gsub vs gsub!, strip vs strip!. Though it’s a common thing to see bang added for “mutators” among the Ruby community - even our API (Entity.clear!. Though the Ruby core is more consistent.
http://dablog.rubypal.com/2007/8/15/bang-methods-or-danger-will-rubyist

Even if you strayed from that and added bang to “dangerous” methods - this one would not be one. As you could simply undo the update and restore it. It’s be dangerous if it did something that was in no way reversible.

[DanRathbun]

@tt_su

It should not be banged. Bangs are supposed to be used for a mutator only if there is a non-mutation version.

Oh that is correct. Forgot that tidbit.

[samyell]

Does anyone know when this reload / update feature might be implemented. I’ve been doing a lot of work on my SU library files to try to eliminat these # issues when brining components in. I thought it was something I had done wrong rather than an error with SU. A component definition update feature would be amazing and the ablity to better handle nested components would also be really useful. The ‘open create collection’ command in the components window means one ends up saving multiple copies of components rather than being able to store 1 component and load it as a nested item into other components that one can then easily save. Realoading all components one by one takes a long time and is open to errors.

[DanRathbun]

Those kinds of “forward looking” statements are not allowed by employees because it is a publicly traded company.

[samyell]

Oh - ok…
Thanks for letting me know Dan. This causes issues for me on a regular basis, I for one would LOVE a fix to this. Seems kind of fundamental to handling components but thats just my opinion!
Cheers
Sam

[bwenz3d]

I would like to take this one step further. I work for a Big Box retailer. All of our layouts have 2D “cells” (you would call them blocks if you were using AutoCAD) for every fixture in our store. We also have a corresponding 3D component in SKP with the same name as the cells in Microstation. What we would like to do is load the 2d Layout into Sketchup, and then quickly replace all of the 2D cells with their 3D counterpart. By doing this all of the 3D components come in at exactly the correct location and orientation. Currently we are achieving this by manually replacing every component by selecting the 2d components, then opening the corresponding 3D library and replacing the instances. Doing this manually is very time consuming as our stores have thousands of components. I do not have the coding skills necessary to automate this, but it seems to me that by loading new definitions from a library you could automatically replace them. This would be a very useful feature. Thanks

Brent

[eneroth3]

A #load method that takes a path as argument would also be useful while your at it.

[DanRathbun]

It doesn’t already ? The API documentation shows that the only argument is a path.

• Sketchup::DefinitionList#load()