Cleanup Vec and Point types

[juliohm]

Fix #91

1. First we convert all occurrences of tuples, SVector, Vec, ... into a single const Vec = SVector to be used consistently across the codebase. Time for us to sign a contract with blood 🩸 to never introduce these variations again :) In this simple exercise I already fixed a conceptual bug where a vector was being used in the place of a point in Base.in(p::Point, r::Rect). Also, bye bye VecTypes, we don't need you anymore.

2. Second we remove the Particle type as it is not really a geometry primitive. Given that the concept of a particle resembles points and vectors, we can get rid of it here as well.

[juliohm]

Simon I really think that this would be the best thing to do in the long run. There are so many hidden bugs and the maintenance burden is all on you because few people can digest the codebase with so many constructors doing much more than just constructing. I'm proposing to become a maintainer if you didn't notice:) it will be much harder for me and others to maintain with these changes not merged. Imagine when we start to apply transformations to these geometries, these constructors may break in non Euclidean manifolds. It is just too hard to keep up with the details and variations. Also as I mentioned we can have nval and other utility functions for the most common use cases? I'm aiming big here with non Euclidean geometries, doing calculations on the sphere directly. These goals won't be achieved with the current state of affairs.

…

On Mon, Sep 28, 2020, 07:29 Simon ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/geometrytypes.jl <#93 (comment)> : > + # muv = uv_mesh(s) + # @test boundbox(Point.(texturecoordinates(muv))) == FRect2D(Vec2f0(0), Vec2f0(1.0)) To be honest, I'm not entirely sure anymore, that we should do this - I always forget how breaking the constructor changes are, and how much I use them... Just imagine this little refactor with you debugging experience you have right now, were you're giving up and referring that to me, and take that by ~20x - that'd approximately be the work that will be put on me if we go through with this. So maybe we should just clean up the implementation a bit, and make sure, that Point & Vec are used consistently! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#93 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3PRNWPNYJO5U2XTNV3SIBQQ5ANCNFSM4R2QDIEQ> .

[SimonDanisch]

I'm sorry but I'm just realistic - and the reality is, that i'm super bottle necked.
Before burdening me with a huge amount of work, I'll need much more evidence than:
"""
Vec3f0(1) == Vec3f0(1, 1,1) is a huge maintenance burden and there are so many hidden bugs with it
"""
I can totally see, that GeometryBasics has a lot of hidden bugs - since it's not tested very well^^
But blaming this on the few methods defined on Vec on top of the methods on SVector seems a bit drastic!

[juliohm]

I meant that the use of vectors and points is being interchanged (incorrectly) and that tuples and vectors have constructors that broadcast entries (incorrectly like above) which causes more bugs. Cleaning these use cases is a first step towards a more robust base package. These convenience constructors should really be convenience functions like we did for boundbox. We were originally abusing the Rect constructor and it took me some time to follow the dispatch of the bounding box use cases. So you are imposing a maintaince burden on we all together when these constructors are used everywhere. Makes sense? It is much easier to track down boundbox and nval in the codebase than search for all call sites of Vec with all equal coordinates.

…

On Mon, Sep 28, 2020, 08:25 Simon ***@***.***> wrote: I'm sorry but I'm just realistic - and the reality is, that i'm super bottle necked. Before burdening me with a huge amount of work, I'll need much more evidence than: """ Vec3f0(1) == Vec3f0(1, 1,1) is a huge maintenance burden and there are so many hidden bugs with it """ I can totally see, that GeometryBasics has a lot of hidden bugs - since it's not tested very well^^ But blaming this on the few methods defined on Vec on top of the methods on SVector seems a bit drastic! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#93 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3OXHJV3T6UT6TITBLTSIBXBHANCNFSM4R2QDIEQ> .

[SimonDanisch]

I agree with the general consent... And don't get me wrong, I'm surely happy about more maintainers & usage.
But it would be much easier on me, to do this step by step. So, e.g. introduce a nval function, while keeping the constructors around.

search for all call sites of Vec with all equal coordinates.

Btw, I'm not sure that's very convincing :D The only refactor that comes to mind where you'd search for that is the one you're currently doing - which I want to avoid... Or are there other refactors, where one would want to replace such constructors?

Btw, I overlooked, that you only changed the filename from boundbox->boundingboxes . Of course, the desire to have unshortened names includes function names, and not only files ;)

[juliohm]

I agree with the general consent... And don't get me wrong, I'm surely happy about more maintainers & usage.
But it would be much easier on me, to do this step by step. So, e.g. introduce a nval function, while keeping the constructors around.

Sorry, I think I wasn't clear about the refactoring plan. The idea is to clean first, make sure every test still pass, and only then introduce @depwarn deprecation warnings correcting for the new behavior. This seems to solve all your concerns about maintenance? It was never part of the plan to create more work on your side.

Let me reiterate again the importance of cleaning this up as soon as possible. Take this PR as an example. It is an evidence that the abuse of the constructor with a single value and possible broadcasting behavior is more harmful than helpful. The only thing I did in this PR was: "let's clean all variations of vectors to a single SVector to be used consistently across the codebase". Now this simple change is causing tests to stack overflow. Why? Very likely the constructors with single values are being used in a place where SVectors are now being passed and somehow things explode. Another evidence that the cleanup is important is finding code like the following in the codebase:

function widths(x::AbstractRange)
    mini, maxi = Float32.(extrema(x))
    return maxi - mini
end

First it assumes that we support AbstractRange to store widths. Second it explicitly converts to Float32 because it is probably only being used in a Makie.jl context with single precision for plotting. So the Vecf0 constructor is being used too much downstream just because it exists. Ideally we would clean these occurrences as well. So to answer your second question:

Or are there other refactors, where one would want to replace such constructors?

Yes, ideally we would clean up all this variations around the concept of a static vector for storing coordinates. The result of not doing this early is that by default we will have more headache fixing bugs downstream. Again, the plan is to do it gradually with deprecation warnings.

Btw, I overlooked, that you only changed the filename from boundbox->boundingboxes . Of course, the desire to have unshortened names includes function names, and not only files ;)

You mean that we should also rename boundbox to boundingbox? I can work on it after this PR here is ready to be merged. For that I would really appreciate your help with the tests above that are failing. As I become more familiar with the concepts in the codebase I will be able to fix more things by myself, but right now there is a lot to work out before going down the path of texturecoordinates and meshes, which is what is currently causing the stack overflow.

[juliohm]

Came back to this PR today. I am trying to fix the 2 tests that failed. I am trying to digest the logic of this function:

function decompose(UVT::Union{UV{T},UVW{T}}, primitive) where {T}
    # This is the fallback for texture coordinates if a primitive doesn't overload them
    # We just take the positions and normalize them
    uv = texturecoordinates(primitive)
    if uv === nothing
        # If the primitive doesn't even have coordinates, we're out of options and return
        # nothing, indicating that texturecoordinates aren't implemented
        positions = decompose(Point, primitive)
        positions === nothing && return nothing
        # Let this overlord do the work
        return decompose(UVT, positions)
    end
    return collect_with_eltype(T, uv)
end

You are trying to compute the UV or UVW coordinates of the vertices in the mesh. However, it seems that the return of texturecoordinates is nothing for the m = mesh(Sphere(Point3f0(0), 1)), and we enter in an infinite recursion where decompose calls itself. Can you please clarify the logic here?

[juliohm]

Ok, all tests are passing now after I fixed another BUG in the source code related to another misuse of point vs. vector. I hope this is enough evidence that we really need to clean things up.

@SimonDanisch could you please share your point of view about these changes after our conversations? Do you plan to merge them into master at some point?

[juliohm]

Should I fork the project and proceed on my own?

[SimonDanisch]

Sorry, I'm a bit on a break right now!

Do you plan to merge them into master at some point?

As I said, I'll be fine with most refactors, that don't mean a lot of work for me!

[juliohm]

As I said, I'll be fine with most refactors, that don't mean a lot of work for me!

Does that mean that all the changes in this PR are acceptable? I just want to double check that you have plans to merge these at some point before I continue with the refactoring of the code and additional fixes.

[juliohm]

I will merge these changes to the cleaned branch that I am working on, and will continue submitting PRs for it in the next days. When you are back from the break, you let me know if the branch is ok to be merged back into master.