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

elastoplastic extension #560

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mathl79
Copy link

@mathl79 mathl79 commented Nov 3, 2022

implementation of the plasticity extension idea to the elasicity plugin.
see: discussion #549

  • new xml config parameter yield stress ("yield" [Pa]). Yield separates the elastic from the plastic domain
  • generation of userdata output: von mieses equivalent stress, curvature, strain(change of curvature)
  • skin/composit stress coloring
  • skin per vertex color added to the datamodel
  • CMAKE config of elasticity.dll changed to destination output folder \plugin\
  • new test model xml file

googletest is not implemented yet --> need help to find sufficient test criterias

skin/composit stress coloring implementation
@google-cla
Copy link

google-cla bot commented Nov 3, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mathl79
Copy link
Author

mathl79 commented Nov 3, 2022

Hi!
I never started a pull request before.
So in order to fulfill the contribution requirements and adopt best practices, i am hoping for some beginner friendly remarks and some guidance.

@quagla quagla self-requested a review November 4, 2022 11:10
@quagla
Copy link
Collaborator

quagla commented Nov 4, 2022

Hi @mathl79 I think you have changed something in the default behavior of the cable? It looks like the test for the exact solution of the cantilever beam fails.

Great that you are visualizing the stresses, this is something I always wanted to do!

@mathl79
Copy link
Author

mathl79 commented Nov 6, 2022

thank you for taking a look.
The string loaded elasticity test model does not have the required "yield" Parameter as it is a elastic test only.
I need to set a infinite yield value in case the "yield" Parameter in the XML is missing at the cable constructor.
In that way, the test model should be loaded and run as expected.

@quagla
Copy link
Collaborator

quagla commented Nov 7, 2022

Do you mean that you updated CantileverIntoCircle in elasticity_test? I don't see that file in your commit (which is probably why the checks are failing)

@mathl79
Copy link
Author

mathl79 commented Nov 8, 2022

The default elastic behavior should no be changed, if the new parameter "yield" is infinite. I missed to implement the default setting and remove the parameter check in this pull request, so the elasticity_test fails.

In the meantime i reworked my version to have a material model with strain hardening.
image

@quagla
Copy link
Collaborator

quagla commented Nov 8, 2022

Great! I think it's fine if you keep pushing new commits to this branch, the PR should be updated automatically.

mathl79 and others added 2 commits November 8, 2022 12:45
inner deformation friction
corrected body coloring in case of multiple cable bodies
@mathl79 mathl79 marked this pull request as ready for review November 8, 2022 14:43
@quagla
Copy link
Collaborator

quagla commented Nov 8, 2022

Thanks for the revised version! My main comments are:

  • Why did you add so much damping to the original cable examples?
  • Did you try the original examples and particularly to run elasticity_test? The default (purely elastic) behaviour has changed and it seems incorrect.
  • More generally, can we break the PR into a visualization and a plasticity PR or would that be impossible?

@mathl79
Copy link
Author

mathl79 commented Nov 8, 2022

I tried to implement a plasticity deformation proportional damping by reducing the torque depending on the change of omega an the Volume.
Now i see, that this additional damping is overlapping with the elastic joint damping. This needs to be separated.

I did not run the tests - but better should have...

So I will change back to the standard plugin models, add extra plastic test models and separate the viscous damping by only applying it, if elements are in plastic domain.

I try to separate the coloring feature from the plasticity as you suggested.
For the coloring, i assume it should be configurable by XML options, right ?
-definition of range (min, max, auto)
-constant RGB color below lower limit
-constant RGB color above upper limit
-color palette
-enable/disable

What else needs to be user controllable ?

Should any scalar properties other than equivalent stress be selectable in future ?

Besides the separation from the plasticity, zhe stress needs to be calculated for the elasticity model anyway, in order to be able to show it.

Btw - what is a good place in the source for the color conversion and palette generation tools, that I have put inside the plugin code from now?

Is this something, that could be used generally or should it stay plugin specific?

@quagla
Copy link
Collaborator

quagla commented Nov 8, 2022

Yes always run tests before committing :)

For the stress visualisation, those are really good questions. I think plugins should decide what to write in your skinuserdata array (as you have done), but if nothing is written, then you have to make sure the current behaviour is maintained. In the future, maybe also the core engine will write other quantities in that array. So, those colouring options could be added either to the visual or more likely to the skin section of the XML, while which quantity to write (e.g. stress) should be an option of the specific plugin. So disabled should be the default behaviour and will be automatically enabled if the plugin writes something.

@yuvaltassa WDYT? Any better idea than using skins for stress visualization?

@mathl79
Copy link
Author

mathl79 commented Nov 9, 2022

Hi,
i did run the tests on my windows machine and all configured tests do pass.
But the PluginTest.CantileverIntoCircle is not even executed. Also the github build check showes, that the windows-2022 build does not run the elasticity tests, but fails at installing the file elasticity.dll into the plugin folder.

I don't know how to correct the config tests.

When building /debugging elasticity.dll, the file is placed into the /bin folder and needs to be copied into the /bin/plugin/ subfolder manually to be recognized as plugin.

I think, that is also linked to the problem above.

@quagla
Copy link
Collaborator

quagla commented Nov 9, 2022

OK that's interesting, there is something broken with Windows then because on the main branch the dll gets copied. @saran-t what are your thoughts?

As mentioned in my previous comment, could you please open a new PR for the visualisation part? You can keep this one open as well and we will get back to it once we complete the visualisation. How does that sound to you? Maybe in the meantime the Windows issues will be fixed.

@mathl79
Copy link
Author

mathl79 commented Nov 9, 2022

https://github.com/deepmind/mujoco/blob/7b7fc09d1fc3d9a970b2123bc41c4d1fbcff35d6/test/CMakeLists.txt#L69-L71

This code excludes the plugin-test from the test config for Windows builds. what is the reason for this?

@quagla
Copy link
Collaborator

quagla commented Nov 9, 2022

Good point, we definitely need to fix Windows. Now I remember we disabled the test because we had some issues loading the DLL. I wonder if your change to CMakeList fixes that? Perhaps you want to open a third PR with only that change and also enabling again the plugin tests for Windows by deleting the lines you quoted in your post?

@quagla
Copy link
Collaborator

quagla commented Nov 11, 2022

My suggestion is that we can start from the visualization PR since that doesn't requires the windows tests.

@quagla
Copy link
Collaborator

quagla commented Nov 17, 2022

Hi @mathl79 I'm pleased to let you know that now you can run plugin tests on Windows as well, see a9781d9

@mathl79
Copy link
Author

mathl79 commented Nov 17, 2022

Perfect, thanks!
I was also at the point to implement the runtime dll loading, as I was not able to link the plugin into the elasticity-test executable at compile time.

@mathl79
Copy link
Author

mathl79 commented Nov 17, 2022

btw. - you can test the branch for the stress skin visuals without the plasticity in my fork.

@quagla
Copy link
Collaborator

quagla commented Nov 17, 2022

Looks like we have to rollback the commit unfortunately, hopefully we'll be able to fix it and activate the windows tests in the next days.

@quagla
Copy link
Collaborator

quagla commented Nov 17, 2022

Good news, Windows is fixed if you rebase from 043ed27 you'll be able to run Windows tests.

For the visualisation, are you going to create a separate PR from your branch or would you prefer if we have a look and apply the changes ourselves?

@mathl79
Copy link
Author

mathl79 commented Nov 17, 2022

Perfect ! Thanks for fixing again!

For me it absolutely ok, if you take my approach as basis to implement the coloring by our own.
I do have the feeling, that my implementation is not generic enough to fit into a public release.

@quagla
Copy link
Collaborator

quagla commented Nov 28, 2022

Hi @mathl79 we are having a look the colouring internally (see #599).

I wanted to ask you what is the rationale for the Von Mises stress computed directly from the curvature stresses without the membrane ones? Does this plastic model fit your needs?

@mathl79
Copy link
Author

mathl79 commented Nov 29, 2022

Hi @quagla,
my intention for the coloring was just to have an visual indicator for the effects of plasticity.
The average bending stress is for my case enough information and the projected surface stress calculation of the skin elements was/is to difficult for me.
Also the coloring on bone level allows non-skin composites to be colorized.

Regarding the plasticity model:
In the meantime i changed my plasticity model in the way, that omega0 remains unchanged, but the internal bending stress / remaining strain is stored. In that way, the hysteresis behavior is working correctly now, which i did not achive with my first attempt.

I hope, that i will have some spare time the next days to finalize my testing and commit that change into my branch.

br,

Mathias

@quagla
Copy link
Collaborator

quagla commented Nov 29, 2022

Great to hear! Do you think you can write a unit test in elasticity_test comparing the reference vs computed hysteresis so any future change will make sure to not break your intended behaviour?

For the skin colouring, after an internal discussion it was decided to do it in a different way so we will have to implement it ourselves. I agree however that colouring geometries (bones) is probably already giving plenty of information so when you are committing your final changes you could perhaps leave the geom colouring but remove the skin part if you can?

By the way, I added a Jet color palette in my branch, maybe you like it https://github.com/deepmind/mujoco/pull/599/files#diff-f8ed3e4597989d57bd3588f824c10fe89b140ca2665a05dd3b8878f2f6f6c23b

@quagla
Copy link
Collaborator

quagla commented Nov 30, 2022

Thanks for the update!

Tip: I think you have to set the parameters of CantileverIntoCircle such that perfect elasticity is obtained without triggering a warning because warnings cause test failures.

[ RUN ] PluginTest.CantileverIntoCircle
D:\a\mujoco\mujoco\test\fixture.cc(43): error: Failed
mju_user_warning: YieldStress is notset: perfect elasticity is assumed !

Copy link
Collaborator

@quagla quagla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments. First of all, great work! However some things must be address before continuing:

  • The original tests and the original examples must be working.
  • A new test for plasticity should be added.
  • It would be great to remove the visualization part from the branch, unless absolutely necessary (we should use the test to validate the results instead).

else {
dYield_UtY = UtStress - YieldStress; //strain hardening stress delta
k_deltaStrain_G = 5 / (UtStrain - YieldStrain_G); //exponential constant for torsional strain hardening
k_deltaStrain_E = 5 / (UtStrain - YieldStrain_E); //exponential constant for bending strain hardening
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these 3 variables unused?

@@ -26,6 +26,29 @@
namespace mujoco::plugin::elasticity {
namespace {


//create color scale palette
void scalar2rgba(float rgba[4], mjtNum Scalar, mjtNum min, mjtNum max) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the main discussion, it would be great to remove the coloring part from this branch.

- stiffness[2]*(omega[2] - omega0[2]) / stiffness[3],
};

void LocalForce(mjtNum qfrc[3], mujoco::plugin::elasticity::Cable::stiffness_ Sel, mujoco::plugin::elasticity::Cable::stiffness_consts_ Sconst,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the full namespace needed?

omega[2] - omega0[2]
};

//actual strain
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you always add a space between // and the comment?

d_omega[2] / (Sel.L_Dz + omega0[2]),
};

mjtNum Yield_stress_top[] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you document this plastic model with e.g. a paper or a link and/or a more detailed explanation in the code?

}
}
else {
mju_warning("YieldStress is notset: perfect elasticity is assumed !");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update elasticity_test such that this warning is not triggered.

<joint kind="main" damping="0.1"/>
<geom type="box" size="0.03535533906 0.02 0.0015" rgba=".8 .2 .1 1" group="3"/>
<skin subgrid="3"/>
<joint kind="main" damping="10"/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I load this model, it immediately explodes.

<geom type="capsule" size=".005" rgba=".8 .2 .1 1"/>
</composite>
<composite prefix="actuated" type="cable" curve="cos(s) sin(s) s" count="41 1 1"
size="0.251327412 .1 4" offset="0.25 0 .05" initial="fixed">
<plugin plugin="mujoco.elasticity.cable">
<!--Units are in Pa (SI)-->
<config key="twist" value="5e8"/>
<config key="bend" value="15e8"/>
<config key="twist" value="1e10"/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I pull the second spring, at some point it looks like the simulation blows up.

@quagla
Copy link
Collaborator

quagla commented Dec 2, 2022

We have added coloring of the geometries using a callback, please see 0d52fea

@quagla
Copy link
Collaborator

quagla commented Mar 1, 2023

Hi @mathl79 are you still interested in submitting this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants