Add methods to get, set, and remove an object's parent(s)

[AndreiCostinescu]

The main aim is to grasp and release objects with robots from C++ code. In addition, the functionality of the added functions can be extended to more than that: Creating complex scenes programmatically, by arranging objects on other objects or connecting objects and part of scenes together, which is why setting a parent object of another is desirable and thus implemented in this feature.

[mmmarinho]

@AndreiCostinescu Thanks for the pull request.

I'm not sure if you meant to start the pull request with the dqrobotics:master, but if you did I'd like to ask for a few changes before your request can be considered.

Please:

1. Add an explanation of what is your aim with this pull request. From the looks of it, you want to grasp and release objects using the cpp-interface-vrep? Preferably tag me and @bvadorno for the proposal of new functionality, given that we need to reflect that in MATLAB and Python.
2. add a minimal working example to showcase your proposal, including the scene file preferably with robots and objects that are default on CoppeliaSim. It must test all the new methods you are proposing.
3. Add a Doxygen-compatible documentation string to all new methods,
4. Follow the style of the rest of the code. For example, two differences easy to spot are the method names in camelcase and the const placement.
5. No changes on the CMakeLists.txt will be allowed for your convenience only. There must be a good reason to change those as there is an entire installer pipeline that depends on those (GithubActions and PPA).
6. No additional files will be allowed if they are simply a matter of taste. For example .gitignore.

[AndreiCostinescu]

Hi, thanks for your reply and change requests! Should I have started this request from the release branch? I went with the default branch in this repository which is the master branch :)

I will work on 2, 3, and 4 :+1:

Concerning 1: is just tagging @mmmarinho and @bvadorno enough? :sweat_smile: And as far as the description goes, you are right: The main aim is to grasp and release objects with robots from C++ code. However, this can be extended to much more than that: Creating complex scenes programmatically, by arranging objects on other objects or connecting objects and part of scenes together, which is why setting a parent object of another made sense to me and why I added this feature.

Concerning 5: I don't think I have changed any functionality in the CMakeLists.txt which would not be backward compatible. The default option in the CMakeLists.txt is still to have the files in submodules as the included files. However, for users which may need another version of the included files other than the one provided in the submodules, I thought that adding this option would make sense. It is a feature, it doesn't remove any of the previous functionality.

Concerning 6: Do you use a .gitignore file and do not commit it or do you not use a .gitignore at all? In any case, I will remove the file from the commit if that's what you want :relaxed:

[mmmarinho]

Hi, thanks for your reply and change requests! Should I have started this request from the release branch? I went with the default branch in this repository which is the master branch :)

The master branch is correct! There was no explanation on the body of the pull request so I erroneously supposed it was made by mistake.

I will work on 2, 3, and 4 👍

Cool!

Concerning 1: is just tagging @mmmarinho and @bvadorno enough? 😅

I did already tag him on my reply so that was OK, thanks! That comment of mine was targeting future pull requests that you and other contributors might propose.

And as far as the description goes, you are right: The main aim is to grasp and release objects with robots from C++ code. However, this can be extended to much more than that: Creating complex scenes programmatically, by arranging objects on other objects or connecting objects and part of scenes together, which is why setting a parent object of another made sense to me and why I added this feature.

Great! Please edit the first comment in your pull request and add this rationale. It will make it easier for us to find the reasoning of the pull request sometime in the future without having to scan through this discussion.

You seem eager to contribute and I really appreciate that! So about 5 and 6 I'll add one argument to each, but I would prefer if we didn't make the discussion on these specific points any longer to not overshadow your core contributions.

Concerning 5: I don't think I have changed any functionality in the CMakeLists.txt which would not be backward compatible. The default option in the CMakeLists.txt is still to have the files in submodules as the included files. However, for users which may need another version of the included files other than the one provided in the submodules, I thought that adding this option would make sense. It is a feature, it doesn't remove any of the previous functionality.

As you might be aware, our support is aimed towards the installable versions. PPA for C++ and PyPI for Python. A quick example to install all available packages in the master branch:

sudo add-apt-repository ppa:dqrobotics-dev/development
sudo apt-get update
sudo apt-get install libdqrobotics*

With that quick preamble in mind, the dqrobotics binaries generated by the PPA use debian/rules and are tied to the specific commit of each submodule in the repository. Basically, the CI is going to build and link against that specific version of each submodule. Upgrading the CoppeliaSim libraries would need me to check a bunch of my (and my students' and collaborators') platforms as the master code runs as-is on many of our robots on a daily basis. So any proposals of upgrading (or otherwise changing the version of) any submodule would probably stall your pull request for an undetermined amount of time and prevent us all from enjoying your core contributions in a timely manner. In addition, If you make changes using another version of the CoppeliaSim libraries and mistakingly commit those trusting that they would work, they might fail at the CI step and, worse, might fail at runtime and that wouldn't be pleasant.

Concerning 6: Do you use a .gitignore file and do not commit it or do you not use a .gitignore at all? In any case, I will remove the file from the commit if that's what you want ☺️

I do not use one at all, because my builds are out of source. You might benefit from using a global one for yourself, as the contents of the one you added to the repository seem like a matter of preference more than something related to this specific repository.

.idea -> Your IDE of choice, I'm guessing something JetBrains
 *-build-* -> If you build in-source and this pattern is somehow useful
 build* -> If you build in-source and this pattern is somehow useful

[AndreiCostinescu]

Concerning 2: minimalWorkingExample.zip

Interestingly enough, it seems that the get_object_parent function needs some reset time before giving the correct result :smile: I think this happens because of the state synchronization not being up-to-date, but I'm not sure how to check...

[AndreiCostinescu]

Thank you for the explanations and links regarding 5 and 6. Didn't know that you can have a global .gitignore :wink:

[mmmarinho]

Concerning 2: minimalWorkingExample.zip

Interestingly enough, it seems that the get_object_parent function needs some reset time before giving the correct result 😄 I think this happens because of the state synchronization not being up-to-date, but I'm not sure how to check...

That usually is an OPMODE issue.

The behavior for the OP_AUTOMATIC (automatic for the end user, not the developer) must be implemented on a case-by-case basis and makes the most sense for methods with a more complicated OP_MODE sequence requirement.

For example, the ones that require the first call to be OP_STREAMING and subsequent calls to be OP_BUFFER: https://github.com/dqrobotics/cpp-interface-vrep/blob/e661139309864e00f7001e17377dcdac9839fdfd/src/dqrobotics/interfaces/vrep/DQ_VrepInterface.cpp#L650

You can refer to the recommended op modes for each method at the CoppeliaSim legacy API docs.

In the case of the ones your are proposing, probably you can make do by setting the default as OP_BLOCKING or OP_ONESHOT and check if that was the behavior that you intended.

PS: Please also remove the this-> to keep the original style.

[AndreiCostinescu]

After trying different combinations between OP_BLOCKING and OP_ONESHOT, there is no difference in the outputs returned... As per the docs, for get_object_parent the op_mode was set to OP_BLOCKING and I altered between OP_BLOCKING and OP_ONESHOT for set_object_parent and remove_object_parents functions. No combination returns the expected output...

[mmmarinho]

I see. You can try communicating with the developers of CoppeliaSim and see if you can get the behavior you want.

Please let me know when you have a release candidate version so that I can start the "official" review.

[AndreiCostinescu]

I've fixed the error by remapping the operation modes inside OP_MODES to the operation modes used by Coppelia's vrep. :ok_hand:
They did not match and I didn't figure that out. You may start the official review now :relaxed:

[AndreiCostinescu]

@mmmarinho, was this pull request merged into the master branch? :relaxed:

[mmmarinho]

@AndreiCostinescu No. After the recent changes you have been making, sadly, there is no way it could.

If you'd be willing to roll this branch back to our last point of discussion (was it d1a752b?), I can reopen this ~issue~ PR.

[mmmarinho]

PS: For this PR to be reviewed we need the MATLAB version of dqrobotics to catch up with all the new functionalities in cpp/python, so it might take a while.

[bvadorno]

@AndreiCostinescu, following up on what @mmmarinho said, there are several reasons for not accepting your pull request. Among them, you are working on an outdated branch. Secondly, you are implementing several things that weren't discussed with the library's moderators (mainly @mmmarinho and me) that would have many side effects.

Before suggesting any new modifications, I advise you to look at DQ Robotics's Contributing section. If not, any pull request without prior discussion will be rejected without further discussions.

Best wishes, Bruno

[AndreiCostinescu]

I have implemented other functionalities that I need in my own code integration. My mistake was that I pushed the changes to the branch which was linked to this issue; I didn't mean to update this pull request.

I have reverted this branch to the discussed version (yes, d1a752b) and I have moved my integration to a different branch.

I have also merged the new changes from master into this branch.

[AndreiCostinescu]

Hi @mmmarinho and @bvadorno, Is there something more you'd like me to change before re-opening the pull request? :blush: