Bug in glued_to?

Few points, in reverse order…

If you iterate the target.attribute_dictionaries and there are none it’s working on nil (rather than the more logical []) and that causes a crash out.
Easy trapped by checking there are dictionaries before copying them over…

The temporary face to glue onto could be located outside of the definition’s bounds [min or max], thereby avoid any over laying; the face could also be defined by just three points, as a triangle…

Doesn’t adding the group from the face run the risk of a splat if the face.parent.entities is not the active_entities context ? No sure how to sidestep that !?

The ‘todo’ for getting already glued instances is also very problematical ?!

1 Like

Thanks for that input. I’ll do a bit more testing.

Not at all I have a refinement for that and almost have it ready to post.

Here is the completed refinement. Check it out and see if you can spot any more issues :slight_smile:

refine ::Sketchup::ComponentInstance do
  class Sketchup::ComponentInstance
    def get_glued_instances
      co = parent.entities.grep(Sketchup::ComponentInstance).find_all do |c|
        (c.glued_to == self) || (!c.glued_to.nil? && c.glued_to.parent == definition && c.parent != definition)
      end
      co
    end

    def glue_to(target)
      instance = self   
      if target.is_a?(Sketchup::Face)
        instance.glued_to = target
        return instance
      end
      
      instance.definition.behavior.is2d = true # "is2d" = "gluable"
      #Don't set if already has "snapping".
      instance.definition.behavior.snapto = SnapTo_Arbitrary unless instance.definition.behavior.snapto
      
      #we need to get a list of components glued to this instance because they will be unglued when the instance is glued to the target
      inst_glued_instances = instance.get_glued_instances

      #close all open component edits so we don't crash SketchhUp
      while Sketchup.active_model.active_path do Sketchup.active_model.close_active end

      #find the model bounds and make sure to place the face beyond that so it doesn't merge with any existing geometry
      max = instance.parent.bounds.max
      corners = [
        Geom::Point3d.new(max.x + 9, max.y + 9, max.z + 10),
        Geom::Point3d.new(max.x + 10, max.y + 9, max.z + 10),
        Geom::Point3d.new(max.x + 10, max.y + 10, max.z + 10)
      ].map { |pt| pt.transform(instance.transformation) }
    
      # If this face merges with other geometry, everything breaks :( .
      # It has to lie loosely in this drawing context though, to be able to it.
      face = instance.parent.entities.add_face(corners)
      instance.glued_to = face
      #Carry over any instances already glued to target.
      faces = [face]
      glued_instances = target.get_glued_instances

      glued_instances.each do |inst|
        max = inst.parent.bounds.max
        corners = 
          [
            Geom::Point3d.new(max.x + 9, max.y + 9, max.z + 10),
            Geom::Point3d.new(max.x + 10, max.y + 9, max.z + 10),
            Geom::Point3d.new(max.x + 10, max.y + 10, max.z + 10)
          ].map { |pt| pt.transform(inst.transformation) }
        face = inst.parent.entities.add_face(corners)
        #we need to get a list of components glued to this instance because they will be unglued when the instance is glued to the target
        child_glued_instances = inst.get_glued_instances
        inst.glued_to = face
        faces.push(face)
        #reglue any child components that have become unglued
        unglued = child_glued_instances.reject { |i| i.glued_to == inst }
        temp_inst = inst
        unglued.each { |i| temp_inst = i.glue_to(temp_inst) }
      end

      group = faces[0].parent.entities.add_group(faces)
      component = group.to_component
      component.definition = target.definition
      component.layer = target.layer
      component.material = target.material
      component.transformation = target.transformation
      component.glue_to(target.glued_to) unless component.glued_to == target.glued_to
      #Copy attributes.
      target.attribute_dictionaries.to_a.each { |dict| dict.keys.each { |key| component.set_attribute(dict.name, key, dict[key]) } }
      #Purge temp definition.
      target.erase!
      Sketchup.active_model.definitions.purge_unused

      #reglue any child components that have become unglued
      unglued = inst_glued_instances.reject { |i| i.glued_to == instance }
      unglued.each { |i| instance = i.glue_to(instance) }
      
      component
    end
  end
end

Since I can only add one like! :heart: :heart::+1: :+1: :+1: :+1: :heart: :heart:

Note that attribute dictionaries can be nested many levels deep. The classification dictionaries are like this.

2 Likes

Serves my purpose since I don’t use nested dictionaries, but thanks for pointing that out.

1 Like

I’d recommend reference a separate method that handles the general case of copying attributes rather than bake it into this method. Also I wouldn’t do any purging as the user may very well have components loaded they want to keep, or the code calling this method may use a reference somewhere to a component that isn’t currently placed in the model.

1 Like

Agree.

Sketchup.active_model.definitions.purge_unused

… should be replaced with …

definitions = Sketchup.active_model.definitions
if definitions.respond_to?(:remove) # SU2018+
  definitions.remove(target.definition) # removes target automatically
else
  target.erase!
end

You could also iterate the DefinitionList before starting and collect the number of unused definitions …

definitions = Sketchup.active_model.definitions
unused = definitions.count {|cdef| cdef.instances.empty? }

… and if unused == 0 then it is safe to do a #purge_unused on the DefinitionList.

Thanks for the suggestions. In my particular case it isn’t of concern.

Sketchup.active_model.start_operation 'New Building', true
Sketchup.active_model.entities.clear!
Sketchup.active_model.definitions.purge_unused
Sketchup.active_model.materials.purge_unused

#purge as needed during operation

Huh? The entities collection and the definitions collection are separate and distinct.

Why do you say “purge at will” during an operation ?

I’m basically saying that I’m starting a new building, and deleting everything existing. I think I also have code to purge materials somewhere in the process. In my particular case I’m starting from scratch. Everything is wrapped in an undoable operation, so I’m guessing that would bring back the deleted definitions (didn’t check).

EDIT: I edited my previous post to clarify.

Ah, okay so you’d force the user to start in a new empty model, and later they could insert it as a component into a site model, etc ?

That’s right. :slight_smile:

1 Like

I’ve now published an improved version of the original proof of concept on GitHub.

One of the differences from Neil’s version is much shorter method. Every identifiable task, e.g. a chunk of code with a “title” comment on top, is a separate method. If included in a bigger library, these would be public methods in other classes/modules, but keeping it simple and making a small standalone lib, I instead made them private.

Regarding grouping things outside the active drawing context, I’ve documented that as a limitation. Any closing of open groups however should be done in the caller method, as there is nothing in the general case saying gluing isn’t used in an active drawing context other than the root drawing context.

For DefinitionsList#remove I have a vague memory it could crash SketchUp when first released, so I rely on the old method of clearing its entities collection.

In addition it would be good to add unit tests, but I’ve been a bit lazy lately.

When it comes to the position of the dummy face, it seems I wrongly assumed it has to contain the component origin. Trying to place the face outside the bounds is a good idea. However I wouldn’t base it on the model bounds max, as this point may very well be the origin depending on where the model is drawn. I typically use the bounds diagonal if I want to offset an arbitrary point inside of the bounds to a position assured to be outside of the bounds.

... a sidebar nitpick about library modules ...

IMO, it makes no sense to call it a library nor put “Lib” in the name, as it isn’t a proper Ruby library module.

The methods should be instance methods (with no self qualifier) and there can also be a module_function statement at the top of the library module, so that module method copies are also created (ie, the library can do “double duty” as a separate library [whose methods are called with qualification from outside] or as a mixin library [and then adding methods to other extension’s modules or classes via the include method].)

Module#module_function :

Creates module functions for the named methods. These functions may be called with the module as a receiver, and also become available as instance methods to classes that mix in the module. Module functions are copies of the original, and so may be changed independently. The instance-method versions are made private. If used with no arguments, subsequently defined methods become module functions.

Just noticed that if the target component is glued to something that will be lost.

I added a line to fix that.

component.glue_to(target.glued_to) unless component.glued_to == target.glued_to

I’m still using this solution, but it’s really a pain, because the original object is deleted. That means the persistentID changes, and any references to the object are inst.deleted? = true

Just got bit by this again. I’m seriously considering rolling out my own ‘gluing’ functionality. Because of the issue with references to deleted components this work around is really difficult to handle all edge cases and prevent things from breaking. Just now when I found something broke I’m not even sure which direction to go because of all the regluing that goes on (recursion), to fix the side effects of the side effects.

:smiley:

1 Like

That is great news. Now to wait a few more years until everyone is on the latest version. Actually with the subscription now the amount of people using legacy versions should eventually decline.