search

rbdl / rbdl-toolkit

Application for visualizing and working with rbdl models
European Union Public License 1.2
19 stars 11 forks source link

fix error with trailing space in animation .csv #30

Open idlebear opened 3 years ago

idlebear commented 3 years ago

Hi there -- when loading animation files, if they have a trailing space on any line, one of two error messages is shown:

Added a small change to ignore spaces when reading the file, though I don't know the code base well enough to know if this has further downstream effects (what are columns?, etc).

ju6ge commented 3 years ago

This is also one of the situations where I am not really satisfied how I had to handle this. To give some context the old meshup had defined it's own custom format to describe animations which basically amounts to a csv file but with the twist of adding an extended header section that could be used to describe which dof of the model each column in the csv is mapped to. But in most cases where people used rbdl nobody bothered to use that header because, the order of the dof does not change between your computation and the visualization. Also the custom parser that meshup is using for this also has some bugs that lead to crashes and debugging that code is also a pain.

So that left me with the choice to reimplement all of that or try something different. I decided that since most people are using plain csv for their computed animations to use a normal csv parser, because at least in that way I can use an already functional csv parser and not worry about introducing more bugs that way (csv in it's base for is already pretty complex to parse if you try to consider all the edge cases). Now in an effort to not break all the animation files that still have that header for historic reasons i also opted to use boost to normalize the old animation format into a csv and adding a warning that the values will not be reordered according to the header.

The other warning you are talking about with the inconsistent amount of columns is produced by the animation plugin, it reads the animation file line by line and adds all the row values to a vector that then gets stored in the class that animates the model. And since a model has a static dof count it throws an error if it detects an animation frame that has a different dof count from the previous animation frames.

Now the place where you added your fix is not the correct one, because what it is doing is stripping all the white space out of the animation file. I think it would be better for avoiding side effects to make sure to only remove trailing white space. That is possible if you go a few lines further down before the call to done=valid_csv_line(cur_line_);. The other place where you could put this fix is in the animation plugin itself in the loadAnimationFile routine there would need to be some code added that detects the trailing white space the situation that leads to it incorrectly detecting more columns.

idlebear commented 3 years ago

Thanks for the explanation! I hear what you're saying about trying to support old code.

Regarding stripping whitespace, is it significant? The code already strips tabs for whatever reason, and to anyone viewing the animation file in a text editor, they are visually identical. I gather the boost library is stripping out all the white space that remains after this input stage and then returning an empty token which the animation plugin is choking on.

Ignore my patch for now -- I'll take another look when I have some time. Right now, I have to figure out an undocumented library for a project due in a couple days. ??

Thanks!

From: Felix Richter notifications@github.com Sent: Wednesday, February 24, 2021 7:08 AM To: ORB-HD/rbdl-toolkit rbdl-toolkit@noreply.github.com Cc: Barry Gilhuly barry.gilhuly@uwaterloo.ca; Author author@noreply.github.com Subject: Re: [ORB-HD/rbdl-toolkit] fix error with trailing space in animation .csv (#30)

This is also one of the situations where I am not really satisfied how I had to handle this. To give some context the old meshup had defined it's own custom format to describe animations which basically amounts to a csv file but with the twist of adding an extended header section that could be used to describe which dof of the model each column in the csv is mapped to. But in most cases where people used rbdl nobody bothered to use that header because, the order of the dof does not change between your computation and the visualization. Also the custom parser that meshup is using for this also has some bugs that lead to crashes and debugging that code is also a pain.

So that left me with the choice to reimplement all of that or try something different. I decided that since most people are using plain csv for their computed animations to use a normal csv parser, because at least in that way I can use an already functional csv parser and not worry about introducing more bugs that way (csv in it's base for is already pretty complex to parse if you try to consider all the edge cases). Now in an effort to not break all the animation files that still have that header for historic reasons i also opted to use boost to normalize the old animation format into a csv and adding a warning that the values will not be reordered according to the header.

The other warning you are talking about with the inconsistent amount of columns is produced by the animation plugin, it reads the animation file line by line and adds all the row values to a vector that then gets stored in the class that animates the model. And since a model has a static dof count it throws an error if it detects an animation frame that has a different dof count from the previous animation frames.

Now the place where you added your fix is not the correct one, because what it is doing is stripping all the white space out of the animation file. I think it would be better for avoiding side effects to make sure to only remove trailing white space. That is possible if you go a few lines further down before the call to done=valid_csv_line(curline);. The other place where you could put this fix is in the animation plugin itself in the loadAnimationFile routine there would need to be some code added that detects the trailing white space the situation that leads to it incorrectly detecting more columns.

- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/ORB-HD/rbdl-toolkit/pull/30#issuecomment-785031062, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACY5FMACL7BINZ3T3APYFULTATT2HANCNFSM4YDAPUQA.

ju6ge commented 3 years ago

The file is only rewritten in the memory of the toolkit not on disk, so it is not that significant. But i know that some people like to use white space separation which is possible because the separator is configurable in the settings. The other reason not to strip white space is that in general csv files may contain strings with white space, though this is not really a concern here since we are just reading numerical values and the sanitizer is only used for the animation files.

I would prefer to just strip the trailing white space of each line, that will break the least things. Maybe the stripping of the tabs could be removed as well, it has been a while since I work on this part of the code so I am not sure if it was required but i don't think so.