SketchUp / api-issue-tracker

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

Removing and creating new SketchUpModel in Layout Document can cause UB #589

Open MrPaulch opened 3 years ago

MrPaulch commented 3 years ago

Bug

  1. Create a SketchUp Document with a Scene with some Entities visible
  2. Use the API to create a Layout Document and insert a Layout::SketchUpModel referencing the aforementioned Scene
  3. Save the Layout Document and inspect it. (Verify the scene is embedded correctly, perform a PDF Export)
  4. Create another Scene viewing another region in the original SketchUp Document
  5. Move at least one Entity from the first to the second Scene
  6. Use the API to open the existing Layout Document.
  7. Remove the Layout::SketchUpModel entity
  8. Add two new Layout::SketchUpModels for each scene (The old one, and the new one)
  9. Save the Layout Document and inspect it. (Verify both new scenes in Layout document, perform a PDF Export)

The Layout file shows both SketchUpModel Entities as they are now in the SketchUp Document. However, the PDF Export of the modified Layout Document is now corrupted. The first Scene contains the Entities from before, the new Scene does not contain anything.

Chose the first or second SketchUpModel and right click on "Update Model Reference" The "corruption" is now visible in Layout itself.

Reproduce with:

The Setup:

# Create some entity at origin
circle_entities = Sketchup.active_model.entities.add_circle(Geom::Point3d.new(0, 0, 0), Geom::Vector3d.new(0, 0, 1), 5, 32)
circle = Sketchup.active_model.entities.add_group(circle_entities)

# Prepare view for scene (zoomed in on entity at origin)
Sketchup.active_model.active_view.zoom_extents

# Create first scene (view on entity at origin)
scene_a = Sketchup.active_model.pages.add("First Page")

# Save SketchUp Document
Sketchup.active_model.save("/tmp/test.skp")

# Create Layout Document
lo_document = Layout::Document.new()

# Create Layout::SketchUpModel
lo_su_bounds = Geom::Bounds2d.new(1, 1, 3, 3)
lo_su_model = Layout::SketchUpModel.new("/tmp/test.skp", lo_su_bounds)
lo_su_model.current_scene = 1
lo_su_model.render if lo_su_model.render_needed?

# Add SketchUpModel Entity to Layout Document
lo_document.add_entity(lo_su_model, lo_document.layers.first, lo_document.pages.first)

# Save Layout Document
lo_document.save("/tmp/test.layout")

# Export a PDF 
lo_document.export("/tmp/test_first.pdf")

#
# Close Sketchup 
#

At this point you should have the files; test.skp, test.layout and test_first.pdf Inspecting test.layout and test_first.pdf shows a circle on the first and single page

Now for the Bug: (Don't forget to close the layout file before proceeding)


# Open SketchUp Document
Sketchup.open_file("/tmp/test.skp")

# Reference first scene
scene_a = Sketchup.active_model.pages.first

# Reference original circle
circle = Sketchup.active_model.entities.first

# Move circle to the "right"
circle.move!(Geom::Vector3d.new(15, 0, 0))

# Prepare view for second scene 
Sketchup.active_model.active_view.zoom_extents

# Create second scene
scene_b = Sketchup.active_model.pages.add("Second Page")

# Create another entity at origin for first scene
triangle_entities = Sketchup.active_model.entities.add_circle(Geom::Point3d.new(0, 0, 0), Geom::Vector3d.new(0, 0, 1), 5, 2)
triangle = Sketchup.active_model.entities.add_circle(Geom::Point3d.new(0, 0, 0), Geom::Vector3d.new(0, 0, 1), 5, 2)

# Save SketchUp Document again
Sketchup.active_model.save()

# Open Layout Document again
lo_document = Layout::Document.open("/tmp/test.layout")

# Reference first Page on Layout Document
lo_page_a = lo_document.pages.first

# Reference existing Layout::SketchUpModel
original_su_model = lo_page_a.entities.first

# Remark: At this point an update_model_reference would be great

# Instead: Remove original Layout::SketchUpModel
lo_document.remove_entity(original_su_model)

# Create second page on Layout Document
lo_page_b = lo_document.pages.add("Second Page")

# Create Layout::SketchUpModel for first scene
lo_su_bounds = Geom::Bounds2d.new(1, 1, 3, 3)
lo_su_model_a = Layout::SketchUpModel.new("/tmp/test.skp", lo_su_bounds)
lo_su_model_a.current_scene = 1
lo_su_model_a.render if lo_su_model_a.render_needed?

# Add first SketchUpModel Entity to first page on Layout Document
lo_document.add_entity(lo_su_model_a, lo_document.layers.first, lo_page_a)

# Create Layout::SketchUpModel for second scene
lo_su_model_b = Layout::SketchUpModel.new("/tmp/test.skp", lo_su_bounds)
lo_su_model_b.current_scene = 2
lo_su_model_b.render if lo_su_model_b.render_needed?

# Add second SketchUpModel Entity to second page on Layout Document
lo_document.add_entity(lo_su_model_b, lo_document.layers.first, lo_page_b)

# Save Layout Document
lo_document.save("/tmp/test.layout")

# Export a PDF 
lo_document.export("/tmp/test_second.pdf")

Now we should have another file called test_second.pdf

Open test.layout Notice a triangle on the first and a circle on the second page This is as expected

Open test_second.pdf Notice a circle on the first and nothing on the second page This is faulty

Suspicion

Layout::Document.remove_entity(...) and Layout::Layers.delete_entities(..., delete_entities=true) do not remove the internaly stored SketchUp Document. Creating a new SketchUpModel Entity in the Layout Document uses the cached SketchUp Document instead of the updated one.

DanRathbun commented 3 years ago

1.5 Save the model to a SKP file.

5.5 Resave the model to the same SKP filepath.

  1. Move at least one Entity from the first to the second Scene.

FTR, SketchUp scene pages do not have an entities collection, but do keep lists of whether entities are visible in their scene. (Meaning that you cannot actually move entities from or to a scene page.)

thomthom commented 3 years ago

Can you explain what "UB" stands for here?

Move at least one Entity from the first to the second Scene

How do you move an entity between scenes in SketchUp?

Do you have a small snippet that reproduce the steps above?

MrPaulch commented 3 years ago

1.5 Save the model to a SKP file.

5.5 Resave the model to the same SKP filepath.

  1. Move at least one Entity from the first to the second Scene.

FTR, SketchUp scene pages do not have an entities collection, but do keep lists of whether entities are visible in their scene. (Meaning that you cannot actually move entities from or to a scene page.)

Moving the entity from one scene to another was just an example. In my use case the scenes had mutually exclusive view ports. I should have put this more simple: The point is to make any change to something visible in a scene. I will clarify this with a code snippet.

MrPaulch commented 3 years ago

Can you explain what "UB" stands for here?

Move at least one Entity from the first to the second Scene

How do you move an entity between scenes in SketchUp?

Do you have a small snippet that reproduce the steps above?

With "UB" I mean "Undefined Behavior" as this bug does not produce an error or a crash, but unexpected and false results. In addition, sometimes after generating the bug, updating the scene in Layout will show another completely unrelated viewport. I could not reliably reproduce the second mentioned behavior.

I've updated my original report to include step by step instructions to reproduce the bug. I hope this helps you identify the problem

sketchupbot commented 3 years ago

Logged as: SKEXT-2867

Fredosixx commented 1 year ago

I can reproduce this type of problem, where Creating a new Layout::SketchUpModel from a Sketchup model refers to the cached information if the Sketchup Model was referred to in the Layout document. In particular the list of scenes is the one of the cached model.