Implement basic types #2
Conversation
| enum class Type : std::size_t { | ||
| boolean, | ||
| number, | ||
| vector_2, |
There was a problem hiding this comment.
do we really need vector2/matrix2/matrix3 ? vector3 and matrix4 suffice, albeit with a little bit of redundant data. i suggest we limit the interpreter to using those 2 types only for the geometry calculations.
There was a problem hiding this comment.
It's not just about redundant data. There are many contexts where it makes sense to restrict yourself to a 2D plane (think offsets, booleans, etc). Having 2D versions of the primitives you need is super-useful, because it prevents you from accidentally doing things like using a 3D curve in a context where a 2D curve is expected.
This may not be very important for us, but it will be very useful for users, since the interpreter can throw a type error if a Curve_3 is used where a Curve_2 is expected. It will also restrict the combinatorial space if we want to mutate existing BOMs.
| new (p) Vector_2(v); | ||
| } | ||
|
|
||
| Vector_2 |
There was a problem hiding this comment.
why do we need the algebraic relations between the vectors and matrices ? I suggest we stick to the existing Matrix library which works, and convert between Vector_3 and Matrix_4 and Vec3 and Mat4.
There was a problem hiding this comment.
So we don't accidentally try to use a Vector in a context where a Matrix is expected? For instance, we should only ever pre-multiply a matrix with a vector, so M * v should be allowed, but v * M should not.
In any case, for the purpose of being able to quickly port the existing NURBS tool functionality to the interpreter, I'm fine with keeping support or the existing Matrix library. As we discussed, my proposal is to not further extend the API for these classes for now, and provide conversion functions between these classes and yours.
There was a problem hiding this comment.
i don't understand your first argument, but it seems moot anyway because of the second argument :)
| { | ||
| assert(v.size() == 3); | ||
| for (std::size_t i = 0; i < 3; ++i) { | ||
| v_[i] = v.begin()[i]; |
There was a problem hiding this comment.
This is an std::initializer_list, not a std::vector. std::initializer_list::at() does not exist.
| for (std::size_t i = 0; i < 3; ++i) { | ||
| for (std::size_t j = 0; j < 3; ++j) { | ||
| serialize(s, m[i][j]); | ||
| } |
There was a problem hiding this comment.
you are serializing here in row-major order, which is confusing, because opengl matrices are column-major, as is the current implementation of the matrix serialization in the nurbstool. was there a specific reason you chose row-major ?
| } | ||
| return Matrix_3(m); | ||
| } | ||
|
|
There was a problem hiding this comment.
as discussed, we won't implement matrix operations other than (de)serialization, so this should go, right?
| struct Type_of<Matrix_3> | ||
| : std::integral_constant<Type, Type::matrix_3> { | ||
| }; | ||
|
|
There was a problem hiding this comment.
I'm not familiar with this Type_of construct, could you explain briefly?
|
|
||
| class Matrix_3 { | ||
| public: | ||
| class Const_row { |
There was a problem hiding this comment.
as we are not implementing matrix math, the proxy class for the row seems overkill.
| matrix_3_array, | ||
| matrix_4_array | ||
| }; | ||
|
|
There was a problem hiding this comment.
we don't need support for (de)serialization of the vec2 and matrix3 (and _array) classes imo.
| new (p) Vector_2(v); | ||
| } | ||
|
|
||
| Vector_2 |
There was a problem hiding this comment.
i don't understand your first argument, but it seems moot anyway because of the second argument :)
| { | ||
| assert(v.size() == 3); | ||
| for (std::size_t i = 0; i < 3; ++i) { | ||
| v_[i] = v.begin()[i]; |
2 participants
This PR should be easier to understand than the previous one.
First, we introduce the enum Type. Type is used to identify types at runtime. This will be necessary for when we introduce the Value type, which can contain a value of a different type at any given time.
Next, we introduce the Type_of template. This template is used to obtain the value of Type corresponding to a given type T. It should be specialized for each type for which we want to obtain run-time type information. This will be necessary for the code generation, where we want to compare the Type of a value on the stack with the Type of the argument expected by a C++ function.
Finally, we introduce some basic types that we want the interpreter to support. This list is far from exhaustive, and we will add more complex types later on, but this set of types is sufficient to exercise all the features we want the interpreter to support.
As a final note, the Const_row class in each Matrix_* class is a so-called proxy class. It's intended use is to allow a matrix to be accessed as if it were a two-dimensional array, but with the added advantage that debug builds will assert if you try to access an element that's not in the matrix.