SketchUp / api-issue-tracker

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

The trim is very time consuming in large model #696

Open LItterBoy-GB opened 3 years ago

LItterBoy-GB commented 3 years ago

Bug Reports

Please include the following:

  1. SketchUp/LayOut Version: Sketchup 2019 pro
  2. OS Platform: windows 10 x64

g1 trim g2

small model : 0.022106 large model : 0.202183

# small
g1 = Sketchup.active_model.entities.find { |ent|
  ent.is_a?(Sketchup::Group) && ent&.material&.name == 'Marc_Wrist'
}

g2 = Sketchup.active_model.entities.find { |ent|
  ent.is_a?(Sketchup::Group) && ent&.material&.name == 'Marc_Shoes3'
}

g3 = Sketchup.active_model.entities.find { |ent|
  ent.is_a?(Sketchup::Group) && ent&.material&.name == 'Marc_Hair'
}
s = Time.now
g1.trim g2
puts "small : #{Time.now - s}"  # small : 0.022106
Sketchup.undo

# large
g1 = Sketchup.active_model.entities.find { |ent|
  ent.is_a?(Sketchup::Group) && ent&.material&.name == 'Marc_Wrist'
}

g2 = Sketchup.active_model.entities.find { |ent|
  ent.is_a?(Sketchup::Group) && ent&.material&.name == 'Marc_Shoes3'
}

g3 = Sketchup.active_model.entities.find { |ent|
  ent.is_a?(Sketchup::Group) && ent&.material&.name == 'Marc_Hair'
}
Sketchup.active_model.start_operation 'copy_make_g3',true
1000.times do |i|
  tr = Geom::Transformation.new(Geom::Point3d.new(0,10*(i+1),0))
  g4 = g3.copy
  g4 = g4.make_unique
  g4.transform!(tr)
end
Sketchup.active_model.commit_operation

s = Time.now
g1.trim g2
puts "large : #{Time.now - s}"  # small : 0.202183

Sketchup.undo

image

DanRathbun commented 3 years ago

What if g1 and g2 are first moved into a temporary group before the trim ? Then the temp group exploded afterward.

thomthom commented 3 years ago

https://forums.sketchup.com/t/the-trim-is-very-time-consuming-in-large-model/171986/11?u=tt_su

It looks to relate to the groups being trimmed being made unique, whereupon SU it looking for a new unique name. When you have a lot of other groups in the model a lot of the definitions start with “Group” and it has to traverse for longer to find a unique name. Logged as an performance issue.

sketchupbot commented 3 years ago

Logged as: SKEXT-3173

DanRathbun commented 3 years ago

@DanRathbun : What if g1 and g2 are first moved into a temporary group before the trim ? Then the temp group exploded afterward.

@LItterBoy-GB said in public forum post ...

_By the way, the function ( add_group ) has the same problem._

@thomthom replied ...

Yea, it also tries to find a unique name.

@LItterBoy-GB also said in public forum post ...

This means that I can avoid the problem by changing the name (no start with “Group”).

@thomthom replied ...

That might work… worth a try.

One problem with naming and a temporary group is that none of the boolean methods nor the Entities#add_group method accept a name string argument for the result.

But the main problem is that the Group#trim method names the definition and instance results "Difference" each time a trim is done (regardless of the starting names) and does not uniquify the instance name, which can result in multiple instances named "Difference" (but they are instances of different definitions.) The first resultant definition gets the name "Difference", and thereafter they are uniquified with #n suffixes.

So the problem prefix is not necessarily "Group" (at least when it comes to the #trim method.)

And no matter what names is used for the result, the entire definition list of names would still be compared, which is where the time is lost.


New arguments cannot be added to these existing methods (boolean and #add_group,) to control the result names ... but I wonder if:

The internals could perhaps just use the Time.now.to_i or Time.now.to_f as a suffix. (Ie, timestamp the results.) The internals would keep a temporary array of definition timestamps during an operation and check it for unique numbers (which should be faster than comparing character strings.) Or, (during the operation) remember the last timestamp used and just increment from it for a new definition suffix.

The result names could combine both the operation name and the timestamp to generate known unique names.