Statement in conversion function (Component to Group) breaks outer loop. Why?

Developers Ruby API
#1

Hello,

I have made a rather straightforward Sketchup Ruby extension to “groupify” contents in Sketchup (i.e. convert from Components to Groups). As such this also makes Dynamic Components into “normal” components without attributes and options.
I can recursively run this on any component and it works just fine.

The idea was to also have the option to select one or more components manually, and run the script on the selection. And here is where I have run into a strange problem. If I select a single component instance everything works, but if I select multiple components the outer loop breaks (only one component from the selection is actually converted). And the outer loop breaks because of a necessary statement in the conversion function.
Actually, If the intst.erase! statement is commented out then everything in the selection is handled, but this is - of course - no option.

The principle is really simple, but for my life I cannot see why the loop breaks.

convert_component_to_group(inst)
{
...
	inst.erase! # <--- This breaks outer loop??? How come?
...
}

sel.each { |inst|
	convert_component_to_group(inst)
}

In the actual code below I have left out the recursive part to focus on the issue at hand.

I would be grateful if someone could point me to the reason why the outer loop breaks for
multiple selected items (component instances). I realize that I am no Ruby expert, so forgive me if my code looks simplistic.

module JoNoS_Extensions
	module JoNoS_Groupify
    
#----------    
    
def self.convert_component_to_group(inst)

  if !(inst.is_a? Sketchup::ComponentInstance)
    return false
  end
	
  puts "Converting component #{inst} to group "
  
  ents = inst.parent.entities
  grp = ents.add_group()
  tmp = grp.entities.add_instance(inst.definition, inst.transformation)
  tmp.make_unique    # Is this necessary???
  grp.layer = inst.layer
  grp.material = inst.material
  grp.locked = inst.locked?
  grp.hidden = inst.hidden?

  inst.erase!  # <--- This breaks outer loop??? How come?
  tmp.explode
 
  return true
end
    
#----------

def self.xtraverse_top(ent, is_recursive)

  if is_recursive
#    xtraverse(ent)
  end
  
  self.convert_component_to_group(ent)

end

#----------
    
def self.groupify_main()
  # Default code, use or delete...
  mdl = Sketchup.active_model # Open model
  ents = mdl.entities # All entities in model
  sel = mdl.selection # Current selection
  puts ""
  
  is_recursive = false
    
  top_ents = sel
  if top_ents.count > 0
    
    # Define everything as one undo operation
    mdl.start_operation "Groupify"
    
    top_ents.each { |top_ent|
      puts("Top node #{top_ent.inspect} of type #{top_ent.typename}" )
      self.xtraverse_top(top_ent, is_recursive)
    }
   
    mdl.commit_operation
  end

  nil
end

  end #module JoNoS_Groupify
end #module JoNoS_Extensions

# Only for testing, comment out otherwise
JoNoS_Extensions::JoNoS_Groupify::groupify_main()
#2

The reason is simple. You are affecting the same collection that you are iterating. This is a big “no-no” in programming logic of any coding language.

In your example, you are removing members of the model.selection singleton collection object at the same time as you are iterating it, and so the loop loses it’s place and becomes confused as to which is the next member to pass into the code block.

Always make sure that you iterate a copy of a collection if you will be removing elements of that collection.

So you may solve your dilemma, by making an array copy of the selection collection and iterate that, ie:

sel.to_a.each { |inst|
	convert_component_to_group(inst)
}
1 Like
#3

NO not needed, in fact it just creates another definition that will need to be cleaned up later.

#4

Hi again,

OK, I can see it now! The erase actually breaks the selection.
Stupid of me that I did not see it before!!

So instead of a sel.each iteration block I can use a while loop to ensure that the entity to be
erased is actually removed from the selection before it is erased

while true do
      top_ent = sel.first
      break if top_ent == nil

      sel.remove(top_ent)
      puts("Top node #{top_ent.inspect} of type #{top_ent.typename}" )
      self.xtraverse_top(top_ent, is_recursive)
end

Anyway, thanks to anyone trying to look into the problem. It is very fortunate that the
sheer process of formulating (for others) a problem issue, gives you yourself a much better
understanding of the problem at hand!
Happy sketching!!!

#5

Thanks. Thought so, but was not sure…
You are unbelievably fast to reply!! Thanks so much!!!

#6

No, you are making it harder than it need be.

Just use the .to_a() method to create an Ruby array copy and iterate the copy instead of the actual Ruby wrapped C++ selection collection. (Again see the example I gave above.)

Then afterward just let Ruby garbage collection “cleanup” the temporary array copy. If you used it within a method, it will go out of scope when the method call is done, and Ruby will GC it automatically.

In the API documentation for most of the Ruby API’s collection classes, you’ll see at the top that the classes will "Includes: Enumerable". The Enumerable module is a core Ruby mixin module that adds many nifty methods into classes that “mix” them in via the Module#include() method.

#7

Ok, I see that now.
Thanks again for sharing wisdom!!

1 Like