Update SurfaceMatchingDialog.rb #113
Conversation
Resolved an issue with the intersect dialog (divided faces were not getting properly written the drawing_interface and were not getting added as OS model surface objects). Also improved the surface matching algorithm. New method uses centroid of the polygon for matching instead of trying to match every single vertex. Testing has been successful thus far, but it could benefit from additional testing.
|
Wow @Ski90Moo, thanks for wading in to the code here! I will take a look at this over the weekend. |
| edges = entity.entities.grep(Sketchup::Edge) | ||
| edges.each {|edge| edge.find_faces} #Heal empty loops (no face) after intersection, this may not be necessary? | ||
|
|
||
| #Ski90Moo:This will find faces with AttributeDictionaries copied from the base face (this is causing the faces to be added to the drawing_interface but a new model surface object is not created because all the copies point to an existing surface object.) Rough work around solution is to delete the dictionary, delete the sketchup entity, and redraw it. This prompts the downstream observer to create a new model surface object and add the sketchup entities to the drawing_interface. There is probably a more direct solution without having to delete and redraw, but I do not fully understand how the observer works. |
There was a problem hiding this comment.
So the idea is basically to trick SketchUp into thinking the surfaces after intersection are new? I think you identified the root cause of the issue but I do wonder if there is a better solution. Do you have a test model you could upload that demonstrates the bad behavior? You can upload it to a comment here if you rename to have a *.txt extension
There was a problem hiding this comment.
Yes, I agree. Utlimately, we need to get the observer to recognize the new surfaces as actually new, unique entities such that is adds them to the drawing_interface register with unique drawing_interface ids and also creates the corresponding OpenStudio surface object. Here is the test osm file:
test.txt
There was a problem hiding this comment.
Thanks @Ski90Moo, I can reproduce the issue with this test model
There was a problem hiding this comment.
Thanks for taking a look at this @macumber.
There was a problem hiding this comment.
Does this issue happen every time for you on this model @Ski90Moo? I am having trouble reproducing it tonight for some reason.
There was a problem hiding this comment.
Ok, that makes sense. I am using 2021 (mainly because of the renaming spaces in inspector issue). I'll do some testing with 2022 and see if it is still an issue. Otherwise, I agree, the better matching should be the focus for now.
There was a problem hiding this comment.
I addition to checking the area before and after equals 3529.2888356448607, I started checking number of surfaces as well:
OpenStudio::Plugin.model_manager.model_interface.openstudio_model.getSurfaces.inject(0) {|sum, s| sum += 1 } returns 96 after intersection (in case of self intersecting polygon). It returns 97 for correct intersection. There are 62 surfaces to begin with.
There was a problem hiding this comment.
I found another surface that can fail to get broken into multiple pieces, unfortunately this occurs in SketchUp 2023 as well.
There was a problem hiding this comment.
And another failed intersection
There was a problem hiding this comment.
Can you send me the test model?
| new_faces = after_faces - before_faces #find the new faces (that just copied the attributes from the base face) | ||
| temp = [] #create array to temporarily store new_faces vertices | ||
| new_faces.each do |face| | ||
| face.delete_attribute('OpenStudio') #delete dictionary attributes from the copied face so it will not delete the OS model object when deleted |
There was a problem hiding this comment.
The problem with this is that you lose all information associated with the OpenStudio object. Also what deletes the OpenStudio object so we don't create new ones?
There was a problem hiding this comment.
I believe the ModelManager tracks and deletes these. Are you getting some that are duplicated or left over?
I thought it would be a concern regarding the surface properties (ie. boundary conditions and sun/wind exposure) getting reset from the original undivided surface. However, I did some testing and remarkably the surface properties carry forward.
|
@Ski90Moo I put together some reduced matching logic similar to yours but trying to cut down on false positives in this PR #115 I think the biggest problem I see is that sometimes the intersect method results in self intersecting polygons like this: It will be some work, but I think we can detect and fix these self-intersecting polygons after the intersection. The biggest lift will be implementing a self-intersecting polygon algorithm like the one discussed here. Ruby implementation here. |
|
Ok, I'll have to spend some time reviewing this. I came across this same issue when researching the intersect solution and stumbled on this thread. It seems that SketchUp has improved their intersecting since then, so I am not sure why we are still getting this issue. I put in the .find_faces method to try to resolve some of these empty and self intersecting polygon issues |
|
Can you send me this test model?
…On Sun, Mar 19, 2023 at 2:00 PM Dan Macumber ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In plugin/openstudio/lib/dialogs/SurfaceMatchingDialog.rb
<#113 (comment)>
:
> @@ -136,13 +136,32 @@ def intersect(selection)
# iterate through spaces to create intersecting geometry
spaces.each do |space|
- entity = space.entity
- entity.entities.intersect_with(true, entity.transformation, entity.entities.parent, entity.transformation, false, selection.to_a)
+ entity = space.entity
+ before_faces = entity.entities.grep(Sketchup::Face) #create array of faces before the intersect
+ entity.entities.intersect_with(true, entity.transformation, entity.entities.parent, entity.transformation, false, selection.to_a)
+ edges = entity.entities.grep(Sketchup::Edge)
+ edges.each {|edge| edge.find_faces} #Heal empty loops (no face) after intersection, this may not be necessary?
+
+ #Ski90Moo:This will find faces with AttributeDictionaries copied from the base face (this is causing the faces to be added to the drawing_interface but a new model surface object is not created because all the copies point to an existing surface object.) Rough work around solution is to delete the dictionary, delete the sketchup entity, and redraw it. This prompts the downstream observer to create a new model surface object and add the sketchup entities to the drawing_interface. There is probably a more direct solution without having to delete and redraw, but I do not fully understand how the observer works.
And another failed intersection
[image: image]
<https://user-images.githubusercontent.com/1327850/226200838-d611250c-8249-416c-b376-7ae40a12a25c.png>
—
Reply to this email directly, view it on GitHub
<#113 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQUKBFF3XSLHVE4Y2LRKCG3W45JTRANCNFSM6AAAAAATSEIXRM>
.
You are receiving this because you were mentioned.Message ID:
<openstudiocoalition/openstudio-sketchup-plugin/pull/113/review/1347558894
@github.com>
--
Best Regards,
Mike Lovejoy P.E.
Helix Energy Partners LLC
Helix, OR
http://www.helix-engineers.net/
<https://deref-mail.com/mail/client/aJLmMsp2k0Y/dereferrer/?redirectUrl=http%3A%2F%2Fwww.helix-engineers.net%2F>
P:541-379-0271
|
|
Sent them by email |
Added beta testing option for intersections and surface matching.
Added beta testing options for intersect and surface matching.
|
@macumber I added an option for beta testing the intersect and surface matching and retained the original methods on the latest changes...sorry, I have no idea what I am doing with Github, can you see the changes I made? |
|
Thank you Dan |
|
@Ski90Moo I moved this to #118 and reorganized the beta code all into one block. I also added a Contributor License Agreement (CLA) action. It requires you to "sign" the CLA before we can merge a PR with your changes on it. You can do this by adding a comment on my PR with the text "I have read the CLA Document and I hereby sign the CLA" |
Resolved an issue with the intersect dialog (divided faces were not getting properly written the drawing_interface and were not getting added as OS model surface objects). Also improved the surface matching algorithm. New method uses centroid of the polygon for matching instead of trying to match every single vertex.
Testing has been successful thus far, but it could benefit from additional testing.
Fixes #75