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

Slim down GeometryBasics and remove all type complexity #173

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

SimonDanisch
Copy link
Member

@SimonDanisch SimonDanisch commented Mar 17, 2022

So... I tried for a very long time to create a Geometry package that serves all use cases and can be used as a foundation for all other Geometry packages without any performance problems for any domain. This never really worked out for a bunch of reasons.
While GeometryBasics was never used that way, all the ideal requirements I aimed for have turned GeometryBasics into a very overengineered package, with long compile times and complex types.
I never really had a lot of time to actually maintain and grow it though, so on top of being overly complex it has been poorly documented. I also tried to integrate lots of types like LineStrings, which I don't know very well and when to use them :D
So, this PR is slimming down GeometryBasics to the very basics that I need in Makie, which should help a lot with compile times and maintainability.

It changes:

  • removes all kind of meta infrastructure, only leaving a simple MetaMesh type that can hold e.g. uv and normals
  • remove table interface
  • less type artistry and instead tries to use the most simple types as possible
  • copies more instead of creating horrible types like StructArray{ReinterpretArray{SubArray{Int64, 1, Vector{Int64}, Tuple{UnitRange{Int64}}, true}}} (this is even shortened :D )
  • Make Point +/-/* Vec consistent

Fixes:

Will not be in the scope of this package anymore:

@SimonDanisch SimonDanisch changed the base branch from master to sd/no-sarray March 17, 2022 20:38
@SimonDanisch SimonDanisch changed the base branch from sd/no-sarray to master March 17, 2022 20:47
@SimonDanisch SimonDanisch reopened this Mar 17, 2022
@evetion
Copy link
Contributor

evetion commented Apr 25, 2022

This is some good cleanup! This is on top of #169 right?

Over at JuliaGeo/GeoInterface.jl#33, we're now (finally) finishing a new Traits based interface. This gets rid of all the required subtyping on GeoInterface.Geometries and I would like to update the GeoInterface implementation in GeometryBasics here as well.

In that context, I see you removed LineString and changed the decomposition in this PR. In GeoInterface we still assume a Polygon decomposes into 1 or multiple LineStrings like instances (exterior ring and any interiors), which themselves decompose into Pointlikes. Do you think we could still make that work in this PR? Would Line be a proper replacement, or is it just two points?

@SimonDanisch
Copy link
Member Author

Yes, I think we can bring it back, if we document a bit better what its supposed to be used for!
I think we should keep them as simple as possible, so maybe something like:

struct LineString{N, T}
   line::Vector{Point{N, T}}
end

struct MultiLineString{N, T}
   lines::Vector{LineString{N, T}}
end

@evetion
Copy link
Contributor

evetion commented May 11, 2022

@SimonDanisch Is it ok to revive https://github.com/JuliaGeometry/GeometryBasics.jl/compare/master...visr:rfc?expand=1 on the current master of GeometryBasics? Or would you like me to open a PR, based on the changes in this PR? Also depends on your expected timeline of this PR?

@SimonDanisch
Copy link
Member Author

No, that sounds great :) Looks like it has 0 dependencies, so that makes things a lot easier ;)
I think master is fine... It's such a small diff, that it should be easy enough to rebase in this PR.

@evetion
Copy link
Contributor

evetion commented Jun 12, 2022

No, that sounds great :) Looks like it has 0 dependencies, so that makes things a lot easier ;) I think master is fine... It's such a small diff, that it should be easy enough to rebase in this PR.

Ok 👍🏻 , I've made a first attempt over at #175.

@jeremiahpslewis
Copy link
Contributor

jeremiahpslewis commented Aug 24, 2022

@SimonDanisch Is this PR still active? From its description, it would solve some issues with adding GeoInterface support to GeoMakie.

@SimonDanisch
Copy link
Member Author

Yeah, still planning to merge it!
Need to first make sure that Makie works though, which is a work in progress: MakieOrg/Makie.jl#2220

Copy link

@termi-official termi-official left a comment

Choose a reason for hiding this comment

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

I think the changes here are a major improvement of the package! It is really way easier to follow what happens now. However, I still have some questions here.

  1. What are the operational and semantic difference between AbstractGeometry and GeometryPrimitive? My understanding on the semantics is that the latter defines "atomic geometry", while the former also includes composites like the meshes. Is this correct or is there more?
  2. What is the minimal example to make a new data type interoperable with Makie? Do we even need the types above for this or is it sufficient to provide a fixed set of dispatches?
  3. Why do the polygons carry the points (in contrast to the mesh carrying a point vector and the polygons carry indices to this vector)?
  4. Is there any documentation on tge interaction between data types from this package and (custom) shaders?

@SimonDanisch
Copy link
Member Author

  1. Good question, I think I introduced that so that one can differentiate between primitives in GeometryBasics and outside... It's been a while, but I think I expected other packages to directly inherit from AbstractGeometry, or their own abstract subtype.
  2. It depends a lot on the type - there isn't a clear interface. Maybe we should have one though.
  3. In OpenGL you almost always have faces + points, so for this refactor I simplified it to always have the indices. For Polygons you shouldn't share points that much... I think a Polygon quickly becomes invalid if you share points?
  4. There isn't really any interaction beyond that MatNf + VecNf can be directly used as Uniforms via GLAbstraction.

@rafaqz
Copy link
Contributor

rafaqz commented Nov 7, 2023

Yes for 3, in most other geometry frameworks points are embedded in the polygon object.

Accessing indexed points is slower due to the indirection, and needs ~50% more memory in the (2d) case that no points are shared. It would only occasionally save ~25% memory when all points are shared between 2 polygons.

@termi-official
Copy link

termi-official commented Nov 7, 2023

Thanks for the elaborations. I will answer separately.

2. It depends a lot on the type - there isn't a clear interface. Maybe we should have one though.

Should we discuss this here on in a separate issue or here?

3. In OpenGL you almost always have faces + points, so for this refactor I simplified it to always have the indices. For Polygons you shouldn't share points that much... I think a Polygon quickly becomes invalid if you share points?

Why should they become invalid? For grid-based simulation technique this is very common. Think e.g. about the surface of a Hexahedral mesh as in the second figure in https://ferrite-fem.github.io/FerriteViz.jl/dev/atopics.html#High-order-fields . I guess it is assumed in the code that the polygon is strictly planar?

Accessing indexed points is slower due to the indirection, and needs ~50% more memory in the (2d) case that no points are shared. It would only occasionally save ~25% memory when all points are shared between 2 polygons.

Indeed a good point. But this changes drastically if we attach more meta information to the polygons.

From reading the code I get the impression that a mesh is required to be triangulated. I.e. we cant have a mesh with polygonal or polytopal elements, right?

@Kevin-Mattheus-Moerman
Copy link

Kevin-Mattheus-Moerman commented May 1, 2024

@SimonDanisch @termi-official @rafaqz would it be worth resuming this discussion or has this gone stale? I am a new user but heavily invested now in GeometryBasics so would welcome and improvements like discussed here.

I can add to the discussion on this point by @termi-official:

Why do the polygons carry the points (in contrast to the mesh carrying a point vector and the polygons carry indices to this vector)?

I can see @rafaqz 's point that in some cases not shipping the points is inefficient (indexing takes time). But in computational methods (physics, engineering, finite element analysis) and geometry processing, this is really annoying and makes coding very inefficient. For instance shared nodes should be shared. If each face/element has its own points it may be convenient for rendering/visualisation, but when adjacent entities should share nodes this approach creates an excess of points that are really not needed. And this would also separate the entities that should really be attached (share points). This causes users to have to merge and split points repeatedly which makes every processing step very inefficient. Below I illustrate this problem with the help of a helpless bunny rabbit, left is imported STL (where each face has its own points), on the right after merging points are properly shared).
So it sounds like we have users/requirements for both of these approaches (1) just indices, 2) ship with points) so perhaps GeometryBasics should support both?

mergevertices_anim.mp4

For context, I am the developer for Comodo and just posted this related issue (before finding this PR): #217

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

6 participants