-
Notifications
You must be signed in to change notification settings - Fork 239
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
[MeshLib] Replacement of element classes with new implementation using TemplateElement #572
Conversation
const MathLib::Vector3 cx (c, x); | ||
const double s = MathLib::scalarProduct(face->getSurfaceNormal(), cx); | ||
delete face; | ||
if (s >= 0) |
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.
Real number comparison.
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.
@TomFischer @rinkk how do you think?
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 can not see a problem here. @wenqing What are your concerns?
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.
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 just check if two vectors are pointing in roughly the same direction. in practice, s
should be nowhere near 0
, if it is the element is degenerated. the test is still correct here because we are testing just for node order so technically s=0
is a valid value, even if other tests will fail because of that. (I am not 100% sure without looking up the theory again, but s=0
almost certainly means this will be a zero volume element)
I think the current implementation has a flexibility in handling different element orders. |
for (unsigned j=0; j<nFaces; ++j) | ||
{ | ||
MeshLib::Face const*const face (dynamic_cast<const MeshLib::Face*>(e->getFace(j))); | ||
const MeshLib::Node x (*(face->getNode(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.
I am curious: Is there a special reason to take node 1 of the face?
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 have no idea. please ask @rinkk
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 is the documentation I wrote:
* Checks if the node order of an element is correct by testing surface normals.
* For 3D elements true is returned if the normals of all faces points away from the centre of
* the element.
* Note: This method might give wrong results if something else is wrong with the element
* (non-planar faces, non-convex geometry, possibly zero volume) which causes the calculated
* center of gravity to lie outside of the actual element
I think I chose node 1 because that way all nodes are used for the test at some point, while for node 0 at least one node in every element type would be used for checking twice and one wouldn't be checked at all. (based on the definition of the _face_nodes
variable)
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.
@rinkk shall I add your comment (about checking node 1) into the source so that we can avoid repating of the same question in the future?
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.
good idea, thank you :) ✅
For what it's worth: everything works fine under windows and with gui. A general comment: *
* 3-----6-----2
* | |
* | |
* 7 9 5
* | |
* | |
* 0-----4-----1
* |
Another question: |
|
thanks for the info |
/** | ||
* Checks if the node order of an element is correct by testing surface normals. | ||
*/ | ||
static bool testElementNodeOrder(const Element* e); |
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 same function exists in Cell class ✅
0f784e3
to
a9635a7
Compare
rebased, changed according to the comments |
#define QUADRULE8_H_ | ||
|
||
#include "MeshLib/MeshEnums.h" | ||
#include "Element.h" |
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.
not needed
maybe it is better to define rule functions in header files so that it helps optimization? |
|
||
/// 2D elements have no faces. | ||
unsigned getNFaces() const { return 0; } | ||
virtual double getArea() const { return _content; } |
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 think getArea
is nowhere used, and there is the new getContent
method in the base class.
Same for the Cell::getVolume
.
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.
@rinkk is it true that getArea is nowhere used?
@endJunction getContent() is not new.
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.
yes, getArea
is not used ... it is one of the methods we included when we first implemented the MeshLib but in reality we always end up using getContent()
instead ...
[MeL] Remove unused include.
bace3bb
to
591e70c
Compare
Follwoing Dima's suggestion, removed T_BASE parameter from TemplateElement class |
Good work! ⏩ |
👍 |
[MeL] Replacement of element classes with TemplateElement<Rule>
OpenGeoSys development has been moved to GitLab. |
This PR is based on discussion in #506 and introduces new implementations of mesh element classes by combining TemplateElement class and element-specific topological/geometrical rules.
Note: It includes two commits in #571