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

Refactoring Vector2D, Vector3D (a.k.a. Vec2, Vec3, etc) #28

Closed
louis-langholtz opened this issue Aug 2, 2017 · 2 comments
Closed

Refactoring Vector2D, Vector3D (a.k.a. Vec2, Vec3, etc) #28

louis-langholtz opened this issue Aug 2, 2017 · 2 comments
Assignees
Labels
Enhancement For suggestions or changes that enhance any part of the project and isn't a bug.

Comments

@louis-langholtz
Copy link
Owner

Expected/Desired Behavior or Experience:

Closer compile-time compatibility with C++17-styled std::array elements. Generic data access via templates to the underlying elements.

Actual Behavior:

Most places in the current code base use .x and .y values as expected as horizontal and vertical values. Some places however (like in the Joint implementation code) overload these semantic notions (of the horizontal and the vertical) with other notions. Like treating them as array elements and offering indexed accessors (see float32& b2Vec2::operator()(int32 i); for example). This overloading has been noted before by users of Erin's Box2D.

Discussion

When I forked Box2D from Erin's codebase which had float32& b2Vec2::operator()(int32 i); in it, I changed this method to instead overload the indexing operator for this functionality and to use a switch statement to access .x and .y components. This served the purpose of using the [] operator which seems more conventional to me for indexed access to data, and served the purpose of avoiding relying the memory layout being compatible. For the sake of discussion, I'll call the former concept a structure of fields and the latter concept an array of elements.

Since then, I've become less fond of the conceptual bias of these structures being collections of fields and more fond of the bias towards being arrays of elements.

For one thing, while .x and .y are just names and access is 1 less character to type than say [0] and [1], I like ascribing meaning to names and enforcing those when possible.

PlayRho could have the existing Vec2 named type being a type alias to Vector2D<Real> like it does now with x and y fields and drop the indexed access support, and gain a separate type like Vec2A which would have indexed access support but not the x and y fields. Alternatively, PlayRho could just drop the .x and .y field access support, and just use an array of elements.

With the former stance, the meanings I see are:

  1. Components are instance of their type.
  2. Components are intended to be contiguous in memory.2.
  3. Components are horizontal and vertical values.

With the latter stance, the meanings I see are:

  1. Components are instances of their type (still).
  2. Components are intended to be contiguous in memory.

So the latter stance loses a meaning. Sort of. The former stance is kind of like a union though in not being so C++ friendly as say C++17 std::variant, because the former stance doesn't ensure that a Vec2 set as an x and y aggregate isn't later accessed as an array nor vice-versa. In other words, in the current implementation, it doesn't enforce three intentions; just two of them. So I ask, why not drop the charade and just go with the latter stance that doesn't impede components [0] and [1] from being used as x and y concepts nor from being used as a pair of impulse concepts either (as is done in the solver code).

For a second and perhaps more important thing however, while .x and .y access is 1 less character to type than say [0] and [1], it seems to me that pure array styled access lends itself more directly to generic templated code. Instead of having code in Vector2D and Vector3D, we could have a base Vector template structure that Vector2D and Vector3D were simply alias templates for. Some code for Vector could then be shared for Vector2D and Vector3D like support for begin and end iterator methods.

This second attribute also seems to have beneficial implications for Mat22 and Mat33 structures which currently aren't templated. But I'd like them to be templates so that we can say have a Matrix22<Mass> for instance which is actually how many joints use Mat22 now but with the types stripped down to base Real values.

This second attribute also seems to have beneficial implications for things like a more natural application of std::inner_product and other standard library functionality.

@louis-langholtz louis-langholtz self-assigned this Aug 2, 2017
@louis-langholtz louis-langholtz changed the title Refactoring Vector2D and Vector3D (a.k.a. Vec2, Vec3, etc) Refactoring Vector2D, Vector3D (a.k.a. Vec2, Vec3, etc) Aug 2, 2017
@louis-langholtz louis-langholtz added the Enhancement For suggestions or changes that enhance any part of the project and isn't a bug. label Aug 2, 2017
@louis-langholtz louis-langholtz added this to the Beta Launch milestone Aug 2, 2017
@louis-langholtz
Copy link
Owner Author

louis-langholtz commented Aug 2, 2017

I believe dropping the field accessors in favor of real array composition will also facilitate use of SSE and other parallel processing capabilities. But if this change is going to happen, it needs to happen IMO before launch so less users had to rewrite their code from using .x and .y to using [0] and [1].

@louis-langholtz
Copy link
Owner Author

This is done now and passes all builds/tests as of commit aad9ebb.

So no more access to Vec2 or Length2D etc via .x or .y.

If a programmer wants to access elements by horizontal or vertical concepts, the template functions GetX and GetY are there now for that. If a programmer wants to access elements by array concepts, the template functions std::get<0> and std::get<1> are there now for this (and implemented for the base Vector template structure). These are all compile-time safe too in that their setup is such that the compiler should output an error if you were to try and do something like access element 4 via code like std::get<4>. I think these should be the recommended ways to access the Vector based types.

Alternatively, the Vector based types can all be accessed like arrays; with subscripts. Like: location[0] or location[1]. Out-of-bounds access as arrays is not guaranteed to be caught by the compiler however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For suggestions or changes that enhance any part of the project and isn't a bug.
Projects
None yet
Development

No branches or pull requests

1 participant