Skip to content

Cleanup Vec and Point types #93

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 3 commits into from
Oct 6, 2020
Merged

Cleanup Vec and Point types #93

merged 3 commits into from
Oct 6, 2020

Conversation

juliohm
Copy link
Member

@juliohm juliohm commented Sep 25, 2020

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
Copy link
Member Author

juliohm commented Sep 28, 2020 via email

@SimonDanisch
Copy link
Member

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
Copy link
Member Author

juliohm commented Sep 28, 2020 via email

@SimonDanisch
Copy link
Member

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
Copy link
Member Author

juliohm commented Sep 28, 2020

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
Copy link
Member Author

juliohm commented Oct 3, 2020

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
Copy link
Member Author

juliohm commented Oct 3, 2020

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
Copy link
Member Author

juliohm commented Oct 6, 2020

Should I fork the project and proceed on my own?

@SimonDanisch
Copy link
Member

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
Copy link
Member Author

juliohm commented Oct 6, 2020

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
Copy link
Member Author

juliohm commented Oct 6, 2020

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.

@juliohm juliohm merged commit 4020b8d into cleanup Oct 6, 2020
@juliohm juliohm deleted the unique-vector branch October 6, 2020 20:47
@juliohm juliohm mentioned this pull request Oct 7, 2020
@juliohm juliohm changed the title [WIP] Cleanup Vec and Point types Cleanup Vec and Point types Oct 8, 2020
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.

2 participants