-
Notifications
You must be signed in to change notification settings - Fork 311
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
[all] Use drawtool everywhere #704
[all] Use drawtool everywhere #704
Conversation
…ivePerformer, EvalPointsDistance, EvalSurfaceDistance
…Field SofaValidation
…id, SofaGeneralSimpleFem, SofaGeneralVisual, SofaMiscFem
ng branch 'upstream/master' into use_drawtool_everywhere # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
[ci-build] |
[ci-build][with-scene-tests] |
void DrawToolGL::drawTriangles(const std::vector<Vector3> &points, const std::vector< Vec4f > &colour) | ||
{ | ||
std::vector<Vector3> normal; | ||
normal.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that if the normal is empty it does nothing ?
Maybe add some comment to explain the logic the declaration of drawTriangles()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, actually if you look closer, this function creates a vector of normals and then calls another function (l. 277) that computes normals if the vector has a null size
{ | ||
glLineWidth(size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will break code that have set the drawLine before.
cannot push/pop the attrib otherwise ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually no since @fredroy set a default value for "size" to one, for functions calling it without specifying the value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The normal convention is to save the attributes before changing them and restoring them to their old value when you are done. In your case you restore it to 1.0 regardless of its initial value this means that calling the function has a side effect on all future opengl drawing operation. It is way better to have side effect free functions so that the one that call them does not have to care about what the function is doing.
@@ -2401,7 +2401,7 @@ void TriangleSetGeometryAlgorithms<DataTypes>::draw(const core::visual::VisualPa | |||
|
|||
const sofa::helper::vector<Triangle> &triangleArray = this->m_topology->getTriangles(); | |||
|
|||
helper::vector<defaulttype::Vector3> positions; | |||
std::vector<defaulttype::Vector3> positions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use helper::vector as much as possible ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could but @fredroy told me that he wanted to make it as independent from SOFA as possible (in this optic, we should also remove the defaulttype calls).
But in general you're right, for the rest of SOFA, we should use helper::vector as much as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear so much inconsistancies :)
@@ -108,6 +108,7 @@ class SOFA_MISC_COLLISION_API TetrahedronModel : public core::CollisionModel | |||
protected: | |||
|
|||
TetrahedronModel(); | |||
void addTetraToDraw(const Tetrahedron& t, std::vector<sofa::defaulttype::Vector3>& tetraVertices, std::vector<sofa::defaulttype::Vector3>& normalVertices); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't the other parameters const ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, by looking into this function, its purpose is actually to build a vector and set new values in it
Remark : I need to fix the test on planeForceField |
p3 += (c - p3)*0.1f; | ||
p4 += (c - p4)*0.1f; | ||
Coord n1, n2, n3, n4; | ||
n1 = cross(p3 - p1, p2 - p1); n1.normalize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use a lambda to reduce the amount of code and make it simpler ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update done
glEnd(); | ||
glPointSize(1); | ||
#endif /* SOFA_NO_OPENGL */ | ||
color = sofa::defaulttype::RGBAColor::magenta(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the following block of code. it would be simpler with a do_whatever(...)
lambda function
} | ||
else | ||
{ | ||
if (d2<edgeInf[i].restlength2*0.9999) | ||
glColor4f(1,0.5f,0,1); | ||
{ | ||
colors.push_back(sofa::defaulttype::RGBAColor(1,0.5, 0,1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put sofa::defaulttype::RGBAColor(1,0.5, 0,1)
and friends as constexpr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update done
TODO: solve conflict and build with scene tests |
…l_everywhere # Conflicts: # modules/SofaMiscForceField/MeshMatrixMass.inl
Data to show the plane is reactivated Names are updated in showPlane and showPlaneScale
Apply the use of drawTool in the open-source core of SOFA
This now makes SOFA a priori independent from OpenGL using the drawTools.
Fix #653
This PR:
Reviewers will merge only if all these checks are true.