search

ubi-agni / human_hand

modular, articulated human hand model
Other
31 stars 6 forks source link

Hide duplicate palm / thumb-proximal taxels #8

Open guihomework opened 3 years ago

guihomework commented 3 years ago

The patches for thumb proximal exist on the palm too (ptmp/ptmd ptip/ptid), to solve the thumb proximal disappearing under the palm surface, but keep the colored markers displayed.

however, in the current situation, the merging of taxels into contacts for markers is problematic because based on the same URDF description that MUST have a group defined.

An idea is to set those markers in a special group called "ignore" or just an empty group maybe, so that the tactile_merger does not consider them when clustering (and maybe also when not clustering, because arrows can "pierce" through the palm even if thumb proximal is underneath)

Additionally, there was a concern about not clustering the palm at all. and produce only individual contact states. So maybe there is a distinction to make with an individual group and an ignored group.

guihomework commented 3 years ago

A quick hack was provided in branch https://github.com/ubi-agni/human_hand/tree/NoDuplicate, until this issue is solved.

rhaschke commented 3 years ago

Each <sensor> has a name, a group, and a list of taxels. The group is only cosmetic: it is used to group sensors in rviz' TactileStateDisplay. It's not used by tactile_merger to compute contacts. The "group" considered for merging is the sensor itself: https://github.com/ubi-agni/tactile_toolbox/blob/melodic-devel/tactile_merger/src/taxel_group.cpp#L75-L83

Thus, my suggestion is to create individual sensors for all palm taxels (actually also proximal thumb taxels, because they are huge as well). The sensors that should be ignored by tactile_merger (not even publishing individual taxel contacts) would be marked by the group="".

guihomework commented 3 years ago

Interesting information, that we needed to have before discussing further.

I understand that the urdf group is currently cosmetic. it permits to activate and deactivate all proximal or all distal sensors, unrelated to which link they belong to. However I think it has more potential, in combination with the other elements like parent_link_name.

For the tactile_merger, there was the question of maybe do an average for the whole hand (so one special group called "hand" in a special urdf dedicated to that grouping). Such a whole hand average requires to compute TF that only the contact display does right now, so I understand it might be a too big change.

Your suggestion to have split sensors is valid, it is possible right now and requires no change.

What about the opposite, "grouping" sensors. Consider a robot with 2 physical tactile sensors on the same link, we cannot group within a single urdf sensor right now due to accessing different channels. Maybe they nicely touch each other physically and one could interpolate across their values to find a nice single contact state too. For instance the iObject+ has a single parent_link with 10 arrays all around its cylinder core, each published as a separate channel (board0, board1, ...) in a tactile_states. so currently, we need to have 10 sensors described in the urdf to access the correct vector of 96 taxels, and this will probably have to stay like this. One can currently group the 10 sensors for the tactile display cosmetic enable/disable but not for the overall force applied on the object, even if they are in the same link and it could make sense to interpolate between them.

This is why I think one should anticipate the future needs, and solve this issue first, but considering the future.

What requires a change is the ignore function at least. It seems empty group is accepted https://github.com/ubi-agni/urdfdom/blob/ros-sensor-refactoring/urdf_parser/src/sensor_parser.cpp#L91 but I try to imagine if it is intuitive for the user.

In any case, now we want to give a second signification (not cosmetic) to a group, we want to use it to decide about merging/clustering/ignoring/not clustering

If I consider future needs, of doing maybe merging across sensors, I would expect an empty group to not be merged but considered as individual sensor and produce a grouped value only for its taxels. This is my intuition but it breaks the cosmetic display as one would need a first_finger_distal group across 2 tip sensors for instance, but a different group name pinky_finger_distal group across 2 tip sensors on the pinky. OR Instead of explicit names, one could decide to merge/cluster per parent link if the group is not empty or has a special value

This should not be too complex to implement in the current tactile_merger as we don't need tf, the parent_link meaning data is all represented in the same link anyway.

guihomework commented 3 years ago

Actually the prefix I propose could define the clustering method, unless enforced by the tactile_merger settings.

rhaschke commented 3 years ago

Learning about all your other future clustering wishes, I think we should separate concerns. The group attribute should be used for cosmetic purposes in TactileStateDisplay only - as right now. On the other hand, one might want to use different clustering techniques on the same URDF, or even within the same ROS session. This asks for a different mechanism to specify clusterings. For the most generic case, I suggest to go for a yaml config file, which explicitly specifies the desired groupings. For compatibility, we would support the two current clustering modes, based on taxels or sensors. The yaml then could configure anything else. To solve the issue for the customer, I suggest to proceed like originally suggested in https://github.com/ubi-agni/human_hand/issues/8#issuecomment-706112464: create individual sensors for each palm taxel. Of course, this will not yet allow hiding the wrench arrows of these taxels.

rhaschke commented 3 years ago

A key question is: Do they want to merge palm taxels or not?

guihomework commented 3 years ago

Please do not consider my comment as "MY" wishes, they are summary of past discussions in our team, and also coming from other people like Gereon and his customers. I did not introduce many new ideas and wanted to raise awareness of potential feature requests before deciding to solve this one too quickly.

Fine to separate concerns as long as you are aware of potential new features before proceeding in this issue, in which I understood you initially proposed to introduce a different meaning of group. You adapted that now.

I am fine with splitting in individual patches to solve the problem quickly but it still leaves a problem. I see the palm contact currently being named palm_link although the sensor name is called palm, I think it comes from the "group" within the merger being identified per link https://github.com/ubi-agni/tactile_toolbox/blob/melodic-devel/tactile_merger/src/merger.cpp#L140 hence even if published separately the contacts for each palm taxel would produce indistinguishable contact_states, even if the sensor name is different (palm1, palm2 etc...) after splitting. I tested, there is still a single palm contact produced, even after splitting, due to same parent link. So requires a change in naming somewhere.

Also, I know the customer would prefer to have the patches that are duplicate not produce any contact state, to avoid having to filter them. So we need to consider a way to ignore those. Empty group was your suggestion, but now that you want to keep group only for the cosmetic display, what is your solution ?

to answer your question, the customer wants to merge contact state in the whole hand. They apparently do so already from existing contacts, but the data is wrong due to thumb proximal and palm having 2 times the same data counted. The quick hack solved the problem, we need to find something similar or at least permit to filter.

rhaschke commented 3 years ago

If the customer is happy with the current quick hack, they should go for this solution, particularly because it's already available. A downside of this quick hack is that the taxels also disappear in TactileStateDisplay. On a longer time scale, the proposed solution will use a yaml file to configure the merger.

guihomework commented 3 years ago

We need to leave the second NoDuplicate branch open for a while then.

To not lose this conversation, maybe one should just convert this issue into something else as the ignore will be handled differently

rhaschke commented 3 years ago

I don't see why we need to keep the NoDuplicate branch open for them. They have a local copy and they can create their own fork. The benefit of github (in contrast to mail conversation) is that nothing is lost. But I will rename the issue to reflect the new content.