Skip to content

Fix the Point concept #101

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

Merged
merged 20 commits into from
Oct 14, 2020
Merged

Fix the Point concept #101

merged 20 commits into from
Oct 14, 2020

Conversation

juliohm
Copy link
Member

@juliohm juliohm commented Oct 9, 2020

Fix #91. This is a final PR within the set of changes discussed in the linked issue.

  • We start by constraining the appropriate behavior for a point in a geometric space, and then consider interactions with vectors. We add a function coordinates with which we can retrieve the underlying coordinates with respect to the canonical Euclidean basis. In a future PR this function should be generalized to work with other coordinate reference systems.
  • We add type aliases for convenience like we did for vectors. Likewise, they are complete instantiations of the type parameters for end users interested in creating coordinates in a given dimension and with given coordinate type. Point2 and Point3 are 3D points with double precision and Point2f and Point3f are the equivalent in single precision.

The PR is work in progress. I will continue fixing the tests and will let you know when it is ready to be reviewed.


- Type aliases are `Point2`, `Point3`, `Point2f`, `Point3f`
"""
struct Point{N,T} <: AbstractPoint{N,T}
Copy link
Member

Choose a reason for hiding this comment

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

Why no inherit from StaticArrays? That would give us all functionality, and you could still get rid of the macro and rework the overloaded functions.
This will make it very unlikely to get merged, since you're dropping tons of functionality

Copy link
Member Author

Choose a reason for hiding this comment

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

What functionality you are missing? It would be helpful to understand why you are resistant.

I appreciate if you can communicate more explicitly the design issues you have in your mind so that we can both work together on a satisfying solution. Currently, we don't have a satisfying point implementation, and I can't see how inheriting from StaticArrays.jl would help.

@juliohm juliohm mentioned this pull request Oct 10, 2020
@juliohm juliohm changed the title [WIP] Fix the Point concept Fix the Point concept Oct 10, 2020
@juliohm juliohm requested a review from SimonDanisch October 10, 2020 16:55
@juliohm
Copy link
Member Author

juliohm commented Oct 10, 2020

This PR is ready for review. All tests are passing. It fixes several bugs related to the vector vs. point issue. Additionally, there are less type conversions between tuples and vectors lying around. It is a massive PR to review, I am sorry.

As discussed above, I don't feel that we should make the point type inherit from static arrays because a point is not a vector, and indexing into a point is not general enough. Right now, we have a trait coordinates(point) that retrieves the underlying static vector of coordinates for computation. This function will be generalized in a future PR to accept a coordinate reference system as a second argument. By default we return the coordinates with respect to a global Cartesian coordinate system centered at zeros.

I left some details for future PRs like for instance the UV, UVW, Normal types, which are related to the concept of Vector and Point. They deserve a separate API because currently they are making the dispatch more complicated than necessary.

Please let me know if this PR is ok, and I can start a second round of PRs to review and cleanup all the geometry primitives.

@juliohm juliohm mentioned this pull request Oct 12, 2020
@juliohm
Copy link
Member Author

juliohm commented Oct 14, 2020

I will assume that the lack of reply means that the changes aren't welcome in master. Thanks for the discussions. I will proceed with my work in a fork as I need to progress more quickly on this front.

@juliohm juliohm merged commit 3919901 into cleanup Oct 14, 2020
@juliohm juliohm deleted the points branch October 14, 2020 12:22
@SimonDanisch
Copy link
Member

I think I made my standpoint very clear ;)
Since I'm very thin on time I can put into this right now, and I'm already completely over subscribed on making Makie work nicely (which currently needs very little work on the Geometry stack priority wise), I can only merge PRs that don't keep me busy for weeks (or even just hours).

If you're willing to upgrade all dependent packages, or not make drastic changes that need changes in 15+ packages (or at least only make them very slowly, with proper deprecation that don't need immediate attention), I'm more than happy to accept your work - as I said, I like the general way this is going, and agree that those concepts need a cleanup. Note, with proper deprecation warnings, we may even be able to crowd source the upgrade process.

If you can soften your stand a bit to focus on making tests and functionality consistent before ripping out core functionality that I use heavily in lots of packages and which, as you found out, isn't easy to refactor, I'm sure we can make this work nicely.

If that's impossible for you, a fork seems to be the only option. Note, that a fork is likely not the end of the world. If you keep in mind, that we may want to merge them back together, we can just integrate your changes at a later point in time, when I have more time for cleaning up the Geometry stack.

@juliohm
Copy link
Member Author

juliohm commented Oct 18, 2020

I forked the project, and am working on a series of bug fixes and improvements in Meshes.jl: https://github.com/JuliaGeometry/Meshes.jl I had to clean up a set of types related to OpenGL, a set of concepts related to graphics, and my intent with this fork is to write a library for computational geometry that is decoupled from use cases in graphics, which unfortunately is not possible with the GeometryBasics.jl design. Because the changes are too breaking and I need to progress more quickly with my own research, it is reasonable to split the efforts for now.

At some point, if users feel that things can be merged back into a single stream of work, that would be great. Meanwhile, if anyone is interested in the fixes and improvements, please feel free to contribute over there.

@SimonDanisch
Copy link
Member

If you fork, please make sure to make the license clear, and not delete commit history... It looks like, even though it was 100% forked from my work, I'm neither in the license nor in the commit history of that "fork"... That's not a fork, that's a copy without any appreciation for former work, which is quite frankly very impolite.

@juliohm
Copy link
Member Author

juliohm commented Oct 18, 2020

Sorry, the repo existed before the forking decision, and the LICENSE file was outdated. I've updated the file to include your name.

@visr
Copy link
Member

visr commented Oct 18, 2020

and not delete commit history

Would be good to address this point as well. For example see how Simon made an effort to preserve the commit history during the integration of MakieLayout into AbstractPlotting in JuliaPlots/AbstractPlotting.jl#449. Now GitHub shows 38 commits all made by you, which will not give an accurate picture of the history of that code.

I really hope you will work towards reintegration of the codebases in the future. I understand it can sometimes be faster to sprint ahead by yourself. Though cleanups can certainly be done, don't underestimate the amount of knowledge and experience that went into creating this codebase. This includes the lessons learned from GeometryTypes.jl. I already experienced myself that I thought one part could be simpler, only to discover the reason why the original implementation is better after trying to make it so.

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.

3 participants