Update Intersect_Masses to work on complex cases

[Larry7707]

This function was rewritten according to Rhino 6 API doc (see here) and tested on Rhino 5. Thus, it is supposed to work on both versions. A further test is needed and minor changes can be made.
The update was aimed to solve the problem of intersections in complicated cases including intersection between floors with different floor plans, multiple intersections between two zones, intersections on multiple surfaces, etc. The function is developed based on intersections between the surface and zone.
Please let me know for any questions. Thanks!

[chriswmackey]

@Larry7707 ,
Thank you again for doing this. I just have a few comments that I have left on your code. Also, If you can export the userobject alongside the edited code as you see in this video series, I can merge this in faster as I won't have to create the userobject myself.

[chriswmackey]

What is it that makes 500 the right number here? Shouldn't this be a while loop if you want it to exit after a certain condition is met?

[Larry7707]

This is to prevent a dead while loop. This loop will break if building and otherBldg have no intersection. I prefer to use for with a large number instead of while to prevent the program to crush.

[chriswmackey]

@Larry7707 ,
Ok but doesn't this mean that the component won't be able to find an intersection when there are more that 500 surfaces to intersect? Why not set it to iterate over all the surfaces of the otherBldg instead of an arbitrary number?

[Larry7707]

Hi @chriswmackey ,
No. the for loop here is just to rerun the intersect procedure to make sure that there is no more intersection between building and otherBldg. It is just for double-check purpose, like an insurance. The for loop below actually does all the intersection work.

for face1 in building.Faces:
            if face1.IsSurface:
                # if it is a untrimmed surface just find intersection lines
                intersectLines = rc.Geometry.Intersect.Intersection.BrepSurface(otherBldg, face1.DuplicateSurface(), tol)[1]
            else:
                # if it is a trimmed surface
                edgesIdx = face1.AdjacentEdges()
                edges = []
                for ix in edgesIdx:
                    edges.append(building.Edges.Item[ix])
                crv = rc.Geometry.Curve.JoinCurves(edges, tol)
                # potential bug - a surface has made of multiple crvs, but not likely
                intersectLines = rc.Geometry.Intersect.Intersection.BrepBrep(rc.Geometry.Brep.CreatePlanarBreps(crv[0])[0], otherBldg, tol)[1]
            temp = rc.Geometry.Curve.JoinCurves(intersectLines, tol)
            joinedLines = [crv for crv in temp if rc.Geometry.Brep.CreatePlanarBreps(crv)]
            if len(joinedLines) > 0:
                newBrep = face1.Split(joinedLines, tol) # return None on Failure
                if not newBrep: continue
                if newBrep.Faces.Count > building.Faces.Count:
                    changed = True
                    building = newBrep
                    break

For your second question, the function used every face from building to intersect with otherBldg as a brep so that it will prevent non-planar problem while minimizing the operation to reconstruct trimmed faces. Rhino BrepFace is a little bit tricky to handle, especially trimmed faces. If we use faces from building to intersect every face from otherBldg, it will be too slow.

[chriswmackey]

@Larry7707 ,
I understand what you have written now but I still think that it should be done with a while loop rather than a for loop. The function you have here will fail if there are more than 500 intersections between one building and another and, while this situation is very rare, there's no need to introduce this arbitrary limitation.

I think you should have a variable that is set to False at the start and, at the end of each iteration of the while loop, you check whether tempBldg.Faces.Count == building.Facea.Count. if it does, then this varibale gets set to True and the while loop ends. So, instead of a for loop over 500, you have "while variable is False: keep looking for intersections"

Once you make this cahnge, I will merge in what you have here.

[Larry7707]

@chriswmackey ,
Yea I think you are right. I have changed the for loop and added a variable done as the flag.
Thanks for your comment and also let me know if there are any other questions when merging.
Happy New Year!

[chriswmackey]

Again, I am not so sure what makes 10e2*len(bldgMassesBefore) a special number here.

[Larry7707]

Same as before. It's just a large number that i will not reach if the loop works correctly. I can get rid of it if you think it is not very clear.

[chriswmackey]

BTW, you can easily update your pull request by committing edits to your branch.

Thank you, again!

[Larry7707]

Thanks for your comment! I have exported and committed the User Object. Also, I added a _compile to allow manual control to run the function to prevent it from running when connecting breps to it (set _compile as True to run). Feel free to change it. Also, any further comments are greatly welcomed.

[chriswmackey]

@Larry7707 ,
Thanks for adding the userobject. When we add a boolean to set the component to run, we conventionally refer to it as "_runIt" and not "_compile". Can you change it to _runIt instead?

Also, you should add a description of that input to the header of the file:
https://github.com/mostaphaRoudsari/honeybee/pull/716/files#diff-50fc51bada647311bbcaf3123d73fb03R35

[Larry7707]

@chriswmackey,
Thanks. Updated according to your comments.

[chriswmackey]

@Larry7707 ,

I apologize for such a late response and thank you for addressing the comment about the number of iterations. The code generally makes sense to me now.

Unfortunately, I was just doing some testing on your new component and I found a case where your component was failing to correctly compute the intersection but the original intersect mass component was able to correctly compute it:

https://drive.google.com/file/d/1tbwlmDD1xLO5mtrEgl-uQJIsn0oPKhss/view?usp=sharing

Was this a result of the recent edits that you made or does your older version also fail to compute this intersection correctly?

Also, if you would not mind using the original component icon in your userobject, that would be helpful.
I am imagining that your component here would replace the older one, assuming we can iron out the kinks and get it to work correctly.

Make sure you use the export method discussed in this video:
https://www.youtube.com/watch?v=n_m5PDqv5pU&list=PLkjfDmSc5Orxq-f_fbmu_HV4tE2ZPIdaC

[Larry7707]

@chriswmackey,
Thanks for the test. I double checked the case and found that was a minor bug that I thought I had fixed but I didn't. Sorry for the confusion. It occurs when a surface is composed of multiple curves. I updated the code and User Component using the method in the video. Let me know if you have other questions.

[chriswmackey]

@Larry7707 ,

I am sorry for such an incredibly late response but I wanted to make sure that I had enough time to run the new component through a bunch of tests to make sure that it performed as expected.

And the results are in! Your component is undeniably an improvement and I can already see that it handles cases of large models much better than the original.

On behalf of all of the people building large models with Honeybee, thank you very much for your contribution, @Larry7707 . And welcome to the contributors page !

Merged!