-
Notifications
You must be signed in to change notification settings - Fork 104
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
Extend SkinDynLib #149
Comments
👍 .
|
@traversaro there is no previous standard, so I think we can safely move |
Update: the
Problem is : now the member that handles this is called "modality", which is not great. Any suggestions? I'm not happy with |
Then, something like:
|
..or even |
Mh... I missed the modalities part. In |
Let me summarize it (with the new names, let us assume the right hand as taken from
As you can imagine, especially in the case of the hands (that have no physical triangles in them -> we cannot group them by simply doing a 12:1 mapping), this mapping is hardcoded, since there is no way for us to reliably perform this task generically. |
The mapping is hardcoded with respect to what? The |
Sorry, I didn't make the point: the hardcoding is made in order to associate each taxel to its respective center (ie in the 12:1 case), for many reasons:
|
Two comments:
I'd propose then the following:
|
Now, whilst we can think about a streamlined convention and overcome this issue, I'd rather keep this approach and try to handle the existing convention the best that we can. Hence, here comes the hardcoding. I hope that other body parts will not be that difficult, but I don't have any idea whatsoever. |
@pattacini your option names are becoming better and better! I don't know if to stick with the latest one or wait for another great idea from your side (not kidding 👍 ) |
😃 I think I won't be more creative than that. |
That was my point: hardcoding this kind of information will be not maintainable in the long term, and even in the short term it would lead to rather complicate code. A possible alternative (but feel free to reject it, given that you are the one actually doing the work 😄 ) is to have a new file for encoding this "patch" center information. For the sake of KISS, it can be simply a txt format similar to the one already used for encoding position and normals. but encoding which taxel is the "patch center" taxel. For example, this could come together with
|
I got you covered: I have already this kind of format in my code in the form of an
This would have the downside of changing the skinManager (not a big deal), but we don't know how much this would affect other sw. |
Perfect! You call it "hardcoding", but I call it "writing model information in a file", that clearly generalizes much better then source code hardcoding, because adding/modifying taxel is just a matter of modifying configuration files. : ) A solution that has less the tast of "hardcoding" would be to introduce in the structure the concept of "patch" that has a center taxel, then the file would map each taxel to its patch.. but I guess it is better to KISS. I like the idea of improving the skin structure configuration file... the only problem is that I don't know if the format you are proposing is a valid .ini file. |
The "valid .ini file" issue can be sorted out (e.g. by putting the ID of the taxel as a key, and the subsequent double(s) as the value of the key:
My main concern was about the fact that probably this might affect some other code that reads from that file. Honestly, I don't think that there is any sw apart from the SkinManager, but who knows. Any comment @pattacini @lornat75 ? |
@alecive as far as you're about to introduce new keys/options in the file, I'd expect there won't be any impact on the existing code. |
@pattacini there will be because the configuration file will move from a non-standard
to a standard
Anyhow, it is a change for the better, hence I would be more than pleased to do that :) |
We could consider that as an improvement of the implementation, thus fine with me. |
Update 1
|
Update 2Now the files in https://github.com/robotology/icub-main/tree/master/app/skinGui/conf/positions are
I will go with the first one, please tell me if you have anything against it. |
Update 3In agreement with @traversaro , we decided to modify the array called
to this:
|
Any impact on the user code? |
|
Good guy :) |
I just skimmed through this discussion; I may have misunderstood some parts so take these purely as comments. In general these seem good improvements. Concerns:
|
Question 1I used Question 2I needed the
To this end, we decided to use only the central taxel of the triangular patch as a reference for its nearby "friends".It might be not useful for most of the users, but I need it :) Furthermore, by default the behavior is (obviously) the Question 3The enum is still there and is kept as it is (although me and @traversaro are still not that happy with that). Andrea used both this enum:
and this array (that is mapped onto that enum):
I simply changed the latter in order to comply with the new names introduced with @traversaro in #57 . So, the string array changed to:
It is not a big deal since the string is not explicitly used in the code (at least the code I reviewed, I don't know about external projects). |
It is worth noting that, albeit its utility seems doubtful, I think that the
|
Hi everyone,
As for the taxel positions files etc., I am concerned about the "invalid taxels". As detailed under http://wiki.icub.org/wiki/Tactile_sensors_%28aka_Skin%29 Reading compensated tactile data, there two types of these:
|
Here are my answers (that were updated in #151, so please refer to the pull request rather than this issue):
Regarding the latter comments about the invalid taxels, in my opinion belong to a completely different issue. It's not the library's fault if we use wrong position files :) Anyhow:
|
patch --> face in computer graphics terminology |
I guess documentation for the new positions/*.txt parameters ( [1] : http://wiki.icub.org/brain/group__icub__skinManager.html |
Agreed. Done in 327d9d0 . |
Currently, there is no standard high-level API for the skin on the iCub. What I'm referring to, is a set of classes that ease and standardize the process of handling single taxels / skin parts / file management. To this end, I would like to collect feedback about a proposal of extending the
skinDynLib
library with some tools I implemented inhttps://github.com/alecive/periPersonalSpace
and might be useful for other users.This is the basic structure with some of the features:
CLASS 1 :
Taxel
- the basic elementBasic Members
Pos
Nrm
FoR
Pos
as translational component;z-axis
coincident withNrm
;x-axis
andy-axis
are chosen arbitrarily on the plane normal toNrm
Optional Members
They are present in my library but I can keep them there and remove them from
icub-main
(I would keep them, though).WRFpos
WRFFoR
FoR
, but in the Root Reference Framepx
Methods
Various constructors,
toString()
,print()
, copy operator, etc), but no method to assign the aforementioned members (see below).CLASS 2 :
skinPart
It is a collection of taxels (it is way too similar to the
SkinPart
enum (cf. http://wiki.icub.org/brain/namespaceiCub_1_1skinDynLib.html#ad9eb09d94574e8020c27676ca5e9bdfc ) already used in the library, but I don't know how to call it better [any help?]).Basic Members
SkinPart name
enum
): something likeSKIN_RIGHT_HAND
string filename
vector<Taxel*> txls
Optional Members
They are used in order to remap the taxels to the center of the triangle they belong to. We used it to reduce an unnecessary high computation (for our [mine and @matejhof 's] tasks).
vector<int> Taxel2Repr
map<unsigned int, list<unsigned int> > Repr2TaxelList
Methods
Various constructors,
toString()
,print()
, copy operator, etcUseful methods
skinGui
txt filesskinGui
txt files because it contains more information)Issues
skinPart
? Does anybody have a better name?Pos
andNrm
for theTaxel
class) or can I add also all of the fancy functionalities I developed over the time for my own purposes (such as, e.g., the mapping between single taxels and the triangle they belong, and so on)?I have the code already done in my repository (i.e. https://github.com/alecive/periPersonalSpace ), so it is only a matter of porting it to
icub-main
. I'll wait a few days for feedback on issue 1 and 2 and I'll start coding on my own (feel free to interrupt me whenever you would like to).The text was updated successfully, but these errors were encountered: