-
Notifications
You must be signed in to change notification settings - Fork 8
No-op fallback for restrict_to_volume! postrender operation
#117
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
base: hughcars/mesh-size-without-physical-groups
Are you sure you want to change the base?
No-op fallback for restrict_to_volume! postrender operation
#117
Conversation
6377159 to
4f02a37
Compare
6e33136 to
0eba4df
Compare
4f02a37 to
fc7e961
Compare
82088bb to
62f0825
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1c6589e to
dff13bb
Compare
…ul with no meshsized left
… simulated area is superset of chips, then not needed and can avoid expensive step
…skip_postrender step, and fix the fragmentation call. Also some more meshsizes for path terminations
… test_schematic_solidmodel for easier subtesting
fdf4034 to
e5fdb03
Compare
gpeairs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two PRs are pretty exciting. Just one comment, and I'll take another look if there are any new changes once you're done testing, but otherwise LGTM
src/solidmodels/render.jl
Outdated
| s::Float64 = mesh_scale() | ||
| trees::Dict{ | ||
| Tuple{Float64, Float64}, | ||
| KDTree{SVector{3, Float64}, Euclidean, Float64, SVector{3, Float64}} | ||
| } = mesh_control_trees() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to use these in the loop below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, must've been a rebase error. I was trying to force away a yellow type safety warning from some unions, but I don't think it helped. I'll check it again and get rid of the debris.
Note: This is based on top of #112 as a required predicate.
Changes as part of #112 significantly reduced the number of entities being rendered to the SolidModel before postrendering. One consequence of this is to reduce the time required for all postrendering operations, as they generally scale superlinearly with number of entities. Additionally, because mesh entities were by nature colocated with many detailed piece of a component, this resulted in a significant amount of nodes, segments and surfaces that were non-reconcilable without a fragment operation. By removing these entities, the "auto union" operation commonly used with
negativeorpositivelayers has been made significantly more powerful, as the union with entity removal reduces the number of total surfaces enormously.This has two consequences: 1) it reduces the number of entities that need to fragment, improving the run time significantly, and 2) it removes constraints on the mesher associated with adhering to these "construction" entities which aren't actually relevant to the physical geometry, thereby improving mesh speed and quality.
This can be most cleanly seen in the QPU17 example, where comparing
mainagainst the results from this branch, when run on an M1 mac using a single thread for meshing.Before:
and after:
Giving a 3.64x speed up in render, 23.15x in 1D meshing, 81.39x in 2D meshing and 2.06x in 3D meshing, for an overall speedup of 4.38x. The most dramatic improvements are in the 1D and 2D meshing, which it's hypothesized comes from a) the huge reduction in total entities, and b) the simplification of those entities that remain.