Removing data alias while preserving scene backward comptibility.

damienmarchal commented 8 years ago

Hi,

I found the usage of data alias more annoying than helpful.
Currently data alias are used to provide backward compatibility with old scenes. The problem is that by doing so it does not indicate to the user that the data field name has changed and that it should change to fix his scenes. The consequence is that we have a lot of old scene that are still using differents name for the same data field.

I would advocate for a more helpful (to the user point of view) approach. This approach consists in saying that the field it is using is now deprecated and provide suggestions to fix his scene. In the following I provide a template of code (taken from the soft-robot plugin) to implement this kind of behavior.

CableModel<DataTypes>::CableModel(MechanicalState* object)
    : InverseProblemConstraint<DataTypes>(object)
    , d_indexDeprecated(initData(&d_indexDeprecated, "index",
                                 "Deprecated, must be replaced by the field name 'indices'"))
    , d_indices(initData(&d_indices, "indices",
                         "List of points connected by the cable (from extremity to actuated point). \n"
                         "If no indices are given, default value is 0. \n"
                         "In case of multiple indices, one point will be actuated \n"
                         "and the others will represent sliding points for the cable."))
{
      d_indexDeprecated.setDisplayed(false);
}

template<class DataTypes>
void CableModel<DataTypes>::init()
  if(d_indexDeprecated.isSet()) {
           msg_warning(this) << "The field of the Cable component named 'index' is now deprecated. "
                                               "To remove this error message, the field 'index' should be replaced by the field 'indices'." ;
     .... depending on whether you want  to force the user to fix his scene (so quitting the init with an error) or if you want a transition phase accepting the two field you just have to duplicate the content of  d_indexDeprecated into d_indices. 
    }

If anyone has an opinion or other approach to the problem it is more than welcome.

D.

matthieu-nesme commented 8 years ago

If the Data was simply renamed (but is having the same role as previously), it is not a big deal to have a alias.

If the Data is no longer used, backward compatibility will have to be done in the 'parse' function (to fill others Data for instance), in that case, it is easy to add a deprecated message. Note that the initial Data should not exist anymore. As an example, you can look at RigidMapping, where the Data 'repartition' was removed, but the 'parse' function still ensures backward compatibility and log a deprecated msg.

damienmarchal commented 8 years ago

To cite Jeff Johson from GUI-Bloopers: "An even more blatant mistake is when designers put the same command onto different menus, but label it differently ... It is fairly serious because users almost always assume that differently labeled commands invoke different function".

" Another problem with putting the same command on multiple name is that it misleads users into believing that the application is more complex than it really is"

On my side I not that Jeff Johson is right and this is exactly the behavior I observed with new-comers to sofa. They are always puzzled and annoyed but the alias and this generates a lot of troubles and frustration.

So I think from an usability point of view this a bigger deal than what we, as developper, are thinking.

To improve the situation maybe we could:

each time an alias is used print a message using msg_info. This message should say:

[INFO] Using of the data "rest_position" which is an alias  (http://thedoctosofa/alias) pointing to the "position' data field. To remove this message you can replace in myscele.xml:30 :
 "<MechanicalObject rest_position='0 1 2 3 '>  with "<MechanicalObject 'position'=

each time an alias is used to for backward compatibility from previous name we should print a message using msg_warning:

[WARNING] Using of the data "rest_position" is now deprecated. 
To remove this message you must update your scene and replace in myscele.xml:30 :  "<MechanicalObject rest_position='0 1 2 3 '>  with "<MechanicalObject 'position'=

indicate in the GUI the alternative names (because some user may search the alias name in the GUI)
we could also totally remove the addAlias from the sofa core (unless for developement convenience) and implement a component that explicitely give data alias in a scene (I don't know if this is hard to implement) Maybe something like:

   <DataAlias src='@position.postion' 'rest_position'>  
   <ComponentAlias src='OGLModel' dest='VisualModel'>

At the beginning of a scene with OGLModel and "rest_position" would be enough to make the alias obvious to the user preserving the convenience of Alias to the developpers.

What do you think about the differnt appraoch ? DM.

hugtalbot commented 8 years ago

Hi @matthieu-nesme and @damienmarchal ,

The use of aliases is indeed very confusing, especially for users or starting devs. On my opinion, this should be removed. Damien, you last option looks the best one to me. Is there any real effectiveness of aliases (other than backward compatibility) ?

thomas-lemaire commented 8 years ago

it may be used in some automatic similar-name-data linking (when using src="@/path/to/component") style of linking data (instead of position="@/path/to/component.position", edges="@/path/to/component.edges",...) I think this style is appreciated for being less verbose, as usual for newbies it is confusing, for experienced users it is a facility

maybe the in between decision could be to clean the useless and confusing aliases ?

thomas

----- Mail original -----

De: "Hugo" notifications@github.com À: "sofa-framework/sofa" sofa@noreply.github.com Envoyé: Mercredi 2 Novembre 2016 11:40:54 Objet: Re: [sofa-framework/sofa] Removing data alias while preserving scene backward comptibility. (#35)

Hi @matthieu-nesme and @damienmarchal ,

The use of aliases is indeed very confusing, especially for users or starting devs. On my opinion, this should be removed. Damien, you last option looks the best one to me. Is there any real effectiveness of aliases (other than backward compatibility) ?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub , or mute the thread .

hugtalbot commented 8 years ago

yes, could be indeed. But I would be ready to make a bet that no alias would remain if we delete the useless ones :smile:

damienmarchal commented 8 years ago

I have implemented a component "MakeAlias" to explicitely specify the Alias of a component (not a data) at the beginning of its scene. It is commited in https://github.com/sofa-framework/sofa/commit/92753c42b7eaa076ace4b3840aa4cac6c18172a7

Example of use:

<xml/>
<Node name="Root">
     <MakeAlias targetcomponent="TPointModel" alias="Point">
     ...
     <Point>  
     <Point>  
     <Point>  
    ...
</Node>

With this approach people that want alias to simplify their scene...have them...but:

there is not alias by default
alias are explict so anyone reading the scene now that the object "TPointModel" will be named "Point"

I'm looking on how to implement something equivalent for Data.

DM.

damienmarchal commented 8 years ago

I finally got a working component "MakeDataAlias" to explicitely specify the data aliases at the beginning of a scene. It is commited in: https://github.com/sofa-framework/sofa/commit/b434f8a0865723a14d70b3a51c9d9d0057bc4e63

<Node name="Root">
     <MakeDataAlias componentname="MechanicalObject" dataname="position" alias="myrest_position">
     ....
    <MechanicalObject myrest_position="1 2 3 4"/>
</node>
̀```

So with MakeAlias and MakeDataAlias we still have alias but in an user friendly way. 

DM.

sofa-framework / sofa

Removing data alias while preserving scene backward comptibility. #35