yade-dev team mailing list archive

[Janek Kozicki <janek_listy@xxxxx>]

Hi,

I looked a bit more into PotentialBlocks, and there are some things
which surprise me, there seem to be some mistakes.

1. we strive to remove all compiler warnings. Currently when yade is
compiled without PotentialBlocks, there are only 6 warnings, and
four of them are caused by external libraries. Problem is that when
compiling with PotentialBlocks there are 198 warnings, especially
about unused variables and narrowing conversion. I've filed a
respective bug report about that:
https://bugs.launchpad.net/yade/+bug/1813401

2. material parameters (like stiffness or heat capacity) are stored
inside Shape class PotentialBlock.hpp. But they should be stored
inside a class derived from Material. My impression (I might be
wrong, in that case please correct me) is that PotentialParticle is
using default materials and default contact law. Then later you
decided that you need a more customized Material (e.g. with heat
capacity) and you have cloned PotentialParticle into PotentialBlock,
instead of adding a Material class. This error propagates down
through all my next remarks.

3. As a result of above mistake there is no IPhys (interaction
physics) functor. Its main purpose  is to calculate the physical
parameters of the contact. Eg. the heat capacity of two
PotentialBlocks that are in contact. Its contents was put into IGeom
functor Ig2_PB_PB_ScGeom.cpp

4. Ig2_PB_PB_ScGeom.cpp and Ig2_PP_PP_ScGeom.cpp are the duplicates of
each other. The code that is a difference between them is about
physics and would better go to the IPhys functor. Please do not
duplicate code. Duplicate code is the worst source of all bugs.
Because someone after a long debugging session finally corrects some
mistake, and believes that it has been corrected. And later the same
bug resurfaces in the duplicated part of the code.

5. PotentialBlock.hpp and PotentialParticle.hpp are duplicates of each
other, the difference between them is what would better go into
Material class

6. because they are duplicates the drawing of class PotentialBlock is
not implemented, while it would work perfectly well with
Gl1_PotentialParticle functor

I am sorry that my remarks are so fundamental in a sense. But this is
all in good will, I hope that fixing these problems would make the
code much more easier to work with for others, and much less prone to
bugs.

After you make a class derived from Material to handle
PotentialBlocks properly, you will see that it will work best with
interaction physics (IPhys) functor which handlles this material. And
suddenly there is no need to duplicate code from
PotentialParticle.hpp into PotentialBlock.hpp. There will be only
PotentialParticle which sometimes can use the newly created material
and also will work with the default material. And this will cause
Gl1_PotentialParticle functor to draw correctly "both" of them.
"Both" in quotes, because there in fact will be inly one, but with
different material.

I hope that you can see that removing a duplicated code will suddenly
make it all simpler.

I understand that you stopped working on this topic and others are
using this code. Maybe Vasileios will want to fix this? In any case
it is good to discuss this, and if possible try to correct it.

After some discussion, maybe we should turn this Object Oriented
design problems into a bug report about PotentialBlcoks.

best regards
-- 
Janek Kozicki                               http://janek.kozicki.pl/  |