-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
Geometry Basics refactor #219
Open
ffreyer
wants to merge
117
commits into
sd/simple-mesh
Choose a base branch
from
ff/refactor
base: sd/simple-mesh
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+2,230
−1,343
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ffreyer
force-pushed
the
ff/refactor
branch
2 times, most recently
from
August 31, 2024 21:11
4199cd5
to
af55972
Compare
This was referenced Sep 4, 2024
5 tasks
- fix normal gen for varying face types - fix normalization of face_normals
Apparently T[] is an AbstractVector here?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a continuation of #173.
I'm planning to keep the more verbose/drastic changes separate from #173 by doing them here so that we can easily do a full reset if need be. Maybe this also makes it easier to see what actually changed. For changes that essentially just fix #173 I'll try to update that pr.
Plans
(Consider the code here pseudocode)
Mesh type
My current plan for the
Mesh
type is roughly:coordinates
are always included so that the Mesh has a Dimension and element typevertex_attributes
includes all other vertex data, e.g. normals, uvs, colors, etc. The current plan is for them to always be anAbstractVector
and maybeAbstractVector{<: Union{T, StaticVector{T}}
(I'll figure out the correct syntax for this later)faces
(or connectivity) includes a vector of facesviews
includes UnitRanges that access sections of faces. This is for compatibility with meshes that define different sub meshes with e.g. different materials, i.e. for Add experimental support for materials in wavefront obj files JuliaIO/MeshIO.jl#95. We're also planning/thinking about adding views when merging meshes which would allow you to separate them again.Mesh wrapper / MetaMesh
As mentioned with
views
not all the data that you may want to bundle with a mesh is vertex data. You may want materials that apply per view, or maybe some mesh-global data like a name or reference to a file. For this we are thinking about adding a very general, simple wrapper:GeometryBasics will probably not touch this metadata at all. It's basically just there are storage which you can use for whatever you want. For obj loading in MeshIO this would be used to store materials, texture references, etc so that Makie can extract them and apply them when plotting a mesh.
MultiFace and per-face Attributesper Attribute facesWavefront obj files allow for faces to carry different indices for positions, normals and texture coordinates. This is useful to avoid duplication of positions/normal/uvs e.g. for per-face normals. As such we want to allow this in GeometryBasics as well. The (second) idea here is to just allow vertex attributes to carry a face vector in the form of a
FaceView
:Each vertex attribute in a Mesh can either be a FaceView or an AbstractVector. In the latter case it will use
faces(mesh)
.MutlIFace Idea
Wavefront obj files allow for faces to carry different indices for coordinates, normals and texture coordinates. We're planning to bring this to GeometryBasics via a
MultiFace
type:Mesh
, theface
would map tocoordinates
and always be thereattrib_faces
would map to the othervertex_attributes
in a meshMeshIO already has Code that essentially transforms MultiFace to normal faces (i.e. addressing each vertex attribute with the same index). We are planning to move that here and simplify MeshIO a little by doing so.
Adding MultiFace also means that we can use it for primitives which provides a solution for per-face Attributes while avoiding duplication. For example a Rect3 could define
coordinates()
ax 8 points,normals()
as 6 vecs, and have faces defined as:MeshIO's remapping code would then take care of the vertex duplication needed for rendering. This would be also be an optional conversion, i.e. a generic
mesh(Rect3(...))
would keep the 8 coordinates, 6 normals and 6 MultiFace's.This may also be helpful for dealing with per-face normal generation and per-face attributes in general. E.g. we could have
decompose(FaceNormal(), mesh)
and some function that merges per-face attributes with a mesh by generating MultiFace's. (I haven't thought that much about this yet.)Using
FaceViews
overMultiFace
has a bunch of smaller benefits:NamedTuple
for vertex attributes, simplifying the mesh typeType Tree
I don't have strong opinions on the type tree and types (beyond Mesh), but some things I'm considering are:
AbstractFace
to Integer elementsMultiType
from structs to aliasconst MultiType = Vector{Type}
Simplex
aGeometryPrimitve
like Rect, Sphere, etc. This would safe us reinventing the wheel with them, I thinkCurrent Type Tree
Concrete types are bold,
Other changes
We also plan to:
(incomplete) TODO
Prototyping;
General:
replace MultiPoints withVector{Point}
etcimprove type tree to avoid explicit checks around MultiFace typesmerges(meshes)
facetype
and add other introspection functions (e.g.hasnormals()
)mesh.views
when switching to a different (vertex) face typetry to allowNormalUVFace{SomeFaceType}
instead ofNormalUVFace{N, T, SomeFaceType}
allowdecompose(MultiFace, multi_faces)
(incomplete type)adddecompose(FT <: AbstractVertexFace, multi_faces) :: Vector{MultiFace{..., FT, ...}}
decompose(facetype, mesh) -> faces, views
and add a new function for that (kept version that takes and produces views)adddecompose(LineFace, multi_faces)