Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: enhancing IFC isolator perf. by applying BVH trees when creating the hiding/isolation subsets #743

Merged

Conversation

Ibrahim5aad
Copy link
Collaborator

This PR is a performance enhancement on the IfcIsolator. Applying BVH trees for the created geometry subsets seems to help with navigating the model (after applying isolation or hiding) seamlessly with the previously encountered lags.

@netlify
Copy link

netlify bot commented Jun 3, 2023

Deploy Preview for bldrs-share ready!

Name Link
🔨 Latest commit 4596dc1
🔍 Latest deploy log https://app.netlify.com/sites/bldrs-share/deploys/647b830402445c0008c710bb
😎 Deploy Preview https://deploy-preview-743--bldrs-share.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@oo-bldrs
Copy link
Contributor

oo-bldrs commented Jun 5, 2023

Hi @Ibrahim5aad, what's the performance delta between the version without BVH trees and the one with?

And does it make sense to expose this as a configurable option via the constructor, or is this something we always want to enable by default?

@Ibrahim5aad
Copy link
Collaborator Author

Ibrahim5aad commented Jun 5, 2023

Hi @Ibrahim5aad, what's the performance delta between the version without BVH trees and the one with?

And does it make sense to expose this as a configurable option via the constructor, or is this something we always want to enable by default?

Hi @oo-bldrs, without setting this flag to true, the hiding/isolation operations cause a significant decrease in the FPS ratio for the viewer sometimes I'm encountering a drop in FPS around 50%, which is causing a very laggy model navigation.

Please check this video where I show by example this drop in FPS and how setting this flag to true is actually helping in better model navigation: https://www.veed.io/view/7aa3df52-7b77-47bb-bbbb-4f38a1003d3c?panel=share

@oo-bldrs oo-bldrs self-requested a review June 9, 2023 10:32
Copy link
Contributor

@oo-bldrs oo-bldrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pablo-mayrgundter
Copy link
Member

@ConorStokes and @nickcastel50 I'm going to start cc'ing you on some work in existing bldrs, esp performance.

Here, you can go to a complex model like:

https://deploy-preview-743--bldrs-share.netlify.app/share/v/gh/IFCjs/test-ifc-files/main/Schependomlaan/IFC%20Schependomlaan.ifc/56/74/108/311035/408692

And select an item by double-clicking it, then use the 'i' key to isolate it. I believe this is now faster by enabling BVH in IFCjs/Three.

I'll go ahead and approve and would appreciate an LGTM just to know you're aware of this track of work as we go.

@Adrian62D
Copy link

@pablo-mayrgundter We experience performance problems at a client with a model spanning 4 buildings. Currently Bldrs is too slow to handle it. Would it be possible to merge this PR so that we can include this in our next release next monday? If needed it could be hidden behind a feature flag.

@ConorStokes
Copy link

@ConorStokes and @nickcastel50 I'm going to start cc'ing you on some work in existing bldrs, esp performance.

Here, you can go to a complex model like:

https://deploy-preview-743--bldrs-share.netlify.app/share/v/gh/IFCjs/test-ifc-files/main/Schependomlaan/IFC%20Schependomlaan.ifc/56/74/108/311035/408692

And select an item by double-clicking it, then use the 'i' key to isolate it. I believe this is now faster by enabling BVH in IFCjs/Three.

I'll go ahead and approve and would appreciate an LGTM just to know you're aware of this track of work as we go.

LGTM

@pablo-mayrgundter pablo-mayrgundter merged commit ecedac3 into bldrs-ai:main Jun 21, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants