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

Caching of preview meshes to improve performance #656

Conversation

IsakNaslundBh
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh commented May 9, 2022

Issues addressed by this PR

Closes #655

Changes the way geometry is rendered from rendering Surface geometry via the specific render method accepting Breps, to instead extract PreviewMeshes that are cached at first preview and then reused to preview.

Also storing preview material rather than only preview colour, as creating those for each frame also slows things down.

This will increase the demand on RAM slightly, but from initial testing with a quite significant amount of obejcts this has not yet proven to be a massive problem.

If it is, we could have a look to instead remove the cached BHoM geometry and Rhino geometry, for them to be extracted on the fly instead, and directly turned to the preview meshes.

Test files

Two testfiles in the folder, first testing that all of our surface types are still visible, second testing the speed. The speed testfile is able to relatively decently handle 100 000 panels with the code on this PR, while it can not handle a fraction of that on main.

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/Grasshopper_Toolkit/%23656-CachingOfPreviewMeshesToImprovePerformance?csf=1&web=1&e=OV1WJr

Changelog

Additional comments

@IsakNaslundBh IsakNaslundBh added the type:feature New capability or enhancement label May 9, 2022
@IsakNaslundBh IsakNaslundBh self-assigned this May 9, 2022
Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

Some minor spelling/formatting changes requested from code review (functionality review not yet performed).

Tagging @alelom as well to take a glance through just to check if this causes any concern for him with the remote compute work he's working on following our nuget discussion this morning @alelom ?

Happy for conversations to be marked resolved if an answer is given @IsakNaslundBh 👍

Grasshopper_UI/Goos/GH_BakeableObject.cs Outdated Show resolved Hide resolved
Grasshopper_UI/Render/CreatePreviewMesh.cs Outdated Show resolved Hide resolved
Grasshopper_UI/Render/CreatePreviewMesh.cs Outdated Show resolved Hide resolved
Grasshopper_UI/Render/CreatePreviewMesh.cs Show resolved Hide resolved
Grasshopper_UI/Render/RenderMaterial.cs Outdated Show resolved Hide resolved
Grasshopper_UI/Goos/GH_BakeableObject.cs Show resolved Hide resolved
Copy link
Contributor

@michaelhoehn michaelhoehn left a comment

Choose a reason for hiding this comment

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

I've compared the code on main and the PR code against the sample scripts and am very happy with the performance improvements. I didn't notice any adverse side effects on memory usage when upping the panel counts quite significantly. The only real difference was noticed when orbiting/panning around the models which provided a significant framerate performance boost. Happy to see this merged 🐛

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance
@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented May 12, 2022

@IsakNaslundBh to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • branch-compliance
  • dataset-compliance
  • copyright-compliance
  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • versioning
  • installer

There are 344 requests in the queue ahead of you.

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

Changes are good, happy with the discussion, great work, thanks @IsakNaslundBh

@alelom unless you desperately need this for your remote computing work, I won't plan to make a nuget out of this until the next beta - but if you do need it desperately then let me know!

@bhombot-ci
Copy link

bhombot-ci bot commented May 12, 2022

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented May 12, 2022

The check documentation-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented May 12, 2022

The check project-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented May 12, 2022

@IsakNaslundBh to confirm, the following checks are now queued:

  • ready-to-merge

There are 359 requests in the queue ahead of you.

@IsakNaslundBh IsakNaslundBh merged commit 97ac635 into main May 13, 2022
@IsakNaslundBh IsakNaslundBh deleted the Grasshopper_Toolkit-#655-CachingOfPreviewMeshesToImprovePerformance branch May 13, 2022 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Caching of preview meshes for surface display
3 participants