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

Changes to ChCollisionModel method prototypes #8

Closed
wants to merge 9 commits into from
Closed

Changes to ChCollisionModel method prototypes #8

wants to merge 9 commits into from

Conversation

rserban
Copy link
Member

@rserban rserban commented Nov 16, 2013

I have made some fixes to the Add***() virtual methods in ChCollisionModel to pass the position vector and rotation matrix through const refs instead of pointers.

This simplifies and cleans up the code and removes the reliance on nonstandard compiler extensions (i.e., it fixes the 'taking address of temporary' linker warnings with gcc or warning C4238 with VS: 'class rvalue used as lvalue').

I have tested this both on Windows (VS2010) and Linux (gcc). Note that some updates would have to be made to the derived class ChModelSphereSet when chrono-mpi is reinstated.

Added a new ChMatrix33 constructor which builds a diagonal matrix with
same elements on the diagonal (useful to construct an identity 3x3 matrix).
Added two utility functions which test if a 3x3 matrix is the identity matrix
and if a vector is the null vector, respectively.
More such changes should be done.  These are the ones required for
the changes to the ChCollisionModel::Add***() function prototypes.
Modified the prototypes of all functions used to specify collision
geometry so that they take const refs to the position and orientation
of the contact shapes (instead of pointers to a ChVector and ChMatrix33,
respectively).

This cleans up the code and addresses compilation warnings regarding
'taking address of temporary' (gcc) or 'class rvalue used as lvalue' (cl).
Pass position and orientation matrix for contact shapes as const refs, not
pointers.
Move defintions of static functions from the header file to the source file
so that these functions are not multiply-defined.
@tasora
Copy link
Member

tasora commented Nov 17, 2013

hi Radu,
thank you for the improvements, I'll merge this stuff later because
now I'm busy

alessandro

Il 16/11/2013 18:47, Radu Serban ha scritto:

I have made some fixes to the Add***() virtual methods in
ChCollisionModel to pass the position vector and rotation matrix
through const refs instead of pointers.

This simplifies and cleans up the code and removes the reliance on
nonstandard compiler extensions (i.e., it fixes the 'taking address of
temporary' linker warnings with gcc or warning C4238 with VS: 'class
rvalue used as lvalue').

I have tested this both on Windows (VS2010) and Linux (gcc). Note that
some updates would have to be made to the derived class
ChModelSphereSet when chrono-mpi is reinstated.


    You can merge this Pull Request by running

git pull https://github.com/rserban/chrono master

Or view, comment on, or merge it at:

#8

    Commit Summary

@rserban
Copy link
Member Author

rserban commented Nov 17, 2013

Sounds good Alessandro.

By default, CMake links executables with full RPATH only in the build tree and
clears the RPATH before installing targets.  This means that, by default, installed
executables will not find the shared libraries (unless these are installed in a default
location, such as /usr/local/lib).

With this change (lifted directly from the CMake wiki), executables are relinked with
the install RPATH.  This allows running the installed executables regardless of the
installation location and without having to change LD_LIBRARY_PATH.
@tasora
Copy link
Member

tasora commented Nov 23, 2013

Hi Radu, I applied your pull request to the chrono origin, but only for the develop branch (from this moment, I'll keep most modifications into the develop branch, and we'll merge develop into master only for main releases).
Please check if everything works fine because this is the first pull request in the chrono repo, and I hope I managed it correctly.
thanks
Ale

@tasora tasora closed this Nov 23, 2013
@tasora
Copy link
Member

tasora commented Nov 23, 2013

Btw, about the ChCollisionModel changes: you are right about the arguments passed to the functions, that took temporary addresses. I always postponed a change to this class because I'd also like to have it more coherent with the way that cylinder, boxes etc. geometries are created in the ChAsset subclasses (those used for visualization).
For instance, it makes not much sense that a box is set in ChBoxShape using three sizes in x y z as a ChVector, whereas in the ChCollisionModel::AddBox() it expects three half sizes, etc etc....
Will need some further improvements...

hmazhar pushed a commit that referenced this pull request Jun 8, 2015
cleaned up CMakeLists for Windows MSVC builds
thepianoboy added a commit that referenced this pull request Sep 16, 2015
shlok191 added a commit that referenced this pull request Apr 11, 2022
shlok191 added a commit that referenced this pull request Apr 14, 2022
* Removing linking libiomp5.lib for vs 2019 deployment

* Implemented build_pychrono.bat code in gitlab-ci.yml

* Fixed .gitlab-ci.yml for windows deployment

* Fixing windows deployment CI script

* Made changes to gitlab-ci.yml file

* Edited .gitlab-ci.yml to fix vs2019 deploy script

* Attempting conda build with conda-forge channel for vs2019 deploy

* Changed bld.bat to fix environment variables

* Edited meta.yaml for vs2019 deployment

* Attempting to run vs2019 deployment with correct vs compiler version

* Attempting to run vs2019 deployment with correct vs compiler version

* Attempt to use vs2019 for windows:vs2019 deployment

* Attempt to fix vs2019 #2

* Attempt to fix vs2019 #3

* Attempt to fix vs2019 #3

* Attempt to fix vs2019 #4

* Attempt to fix vs2019 #4

* Attempt to fix vs2019 #5

* Change windows conda compiler

* Attempt to fix vs2019 #6

* Attempt to fix vs2019 #7

* Attempt to fix vs2019 #8

* Attempt to fix vs2019 #9

* Attempt to fix vs2019 #10

* Attempt to fix vs2019 #11

* Attempt to fix vs2019 #12

* Attempt to fix vs2019 #12

* Attempting vs2019 deployment for windows without irrlcith

* Attempt to fix vs2019 #13

* Attempt to fix vs2019 #14

* Attempting to get vs2019 deployment to pick correct location for Irrlicht

* Revert "Attempting to get vs2019 deployment to pick correct location for Irrlicht"

This reverts commit bea088d.

* Attempt to fix vs2019 #15
shlok191 added a commit that referenced this pull request Apr 28, 2022
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