-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Conversation
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>
.
|
I'm sorry but I'm just realistic - and the reality is, that i'm super bottle necked. |
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>
.
|
I agree with the general consent... And don't get me wrong, I'm surely happy about more maintainers & usage.
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 |
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 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
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.
You mean that we should also rename |
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 |
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? |
Should I fork the project and proceed on my own? |
Sorry, I'm a bit on a break right now!
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. |
I will merge these changes to the |
Fix #91
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 inBase.in(p::Point, r::Rect)
. Also, bye byeVecTypes
, we don't need you anymore.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.