Skip to content
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

Large 64bit integers are corrupted #28

Closed
jan-ehrenberger opened this issue Sep 14, 2021 · 4 comments
Closed

Large 64bit integers are corrupted #28

jan-ehrenberger opened this issue Sep 14, 2021 · 4 comments
Labels
wontfix This will not be worked on

Comments

@jan-ehrenberger
Copy link

The ros1_parser can't handle large 64bit integers because it converts them to double. This corrupts values larger than 1<<53.

For example a uint64 value 2343141149457253134 in the ros bag is shown as 2343141149457253376 in the PlotJuggler.

The problematic line is here:

// if( raw_value >= (1l<<53) ){

@facontidavide facontidavide added the wontfix This will not be worked on label Sep 14, 2021
@facontidavide
Copy link
Contributor

This is a well-known problem with NO SOLUTION in the current implementation of PlotJuggler.

@facontidavide
Copy link
Contributor

note that the limiting factor is QWT itself, which supports only doubles. Solving this issue would be incredibly complex

@jan-ehrenberger
Copy link
Author

Thanks for the info. Yes, you are right. It seems that PlotJuggler uses doubles everywhere.
For my purposes I created a workaround that stores the values as strings, but I do understand it has some downsides. The stored values then can't be used for plotting.

diff --git a/plugins/ros1_parsers/ros1_parser.cpp b/plugins/ros1_parsers/ros1_parser.cpp
index e12390c..7477188 100644
--- a/plugins/ros1_parsers/ros1_parser.cpp
+++ b/plugins/ros1_parsers/ros1_parser.cpp
@@ -51,26 +51,26 @@ bool IntrospectionParser::parseMessage(MessageRef serialized_msg, double& timest
 
     if( it.second.getTypeID() ==  RosIntrospection::BuiltinType::UINT64 )
     {
-        uint64_t raw_value = it.second.extract<uint64_t>();
-        //      if( raw_value >= (1l<<53) ){
-        //        TODO: warn the user?
-        //      }
-        value = static_cast<double>(raw_value);
+      // TODO: Store the 64bit integer values properly.
+      uint64_t raw_value = it.second.extract<uint64_t>();
+      auto& series = _plot_data.getOrCreateStringSeries( key );
+      series.pushBack({ timestamp, std::to_string(raw_value)});
+      continue;
     }
     else if( it.second.getTypeID() ==  RosIntrospection::BuiltinType::INT64 )
     {
-        int64_t raw_value = it.second.extract<int64_t>();
-        //      if( raw_value >= (1l<<53) ){
-        //        TODO: warn the user?
-        //      }
-        value = static_cast<double>(raw_value);
+      // TODO: Store the 64bit integer values properly.
+      int64_t raw_value = it.second.extract<int64_t>();
+      auto& series = _plot_data.getOrCreateStringSeries( key );
+      series.pushBack({ timestamp, std::to_string(raw_value)});
+        continue;
     }
     else if( it.second.getTypeID() ==  RosIntrospection::BuiltinType::STRING ) {
-        // special case for strings
-        auto str = it.second.extract<std::string>();
-        auto& series = _plot_data.getOrCreateStringSeries( key );
-        series.pushBack({ timestamp, str});
-        continue;
+      // special case for strings
+      auto str = it.second.extract<std::string>();
+      auto& series = _plot_data.getOrCreateStringSeries( key );
+      series.pushBack({ timestamp, str});
+      continue;
     }
     else{
       value = it.second.convert<double>();
-- 

@facontidavide
Copy link
Contributor

facontidavide commented Sep 14, 2021

Potentially a better solution (long term, not planning to do it) is to modify the way internal storage is done to use Any type everywhere and cast the value accordingly. The QWT visualization will still be wrong, but at least the number on the 2nd column of the left panel would be correct.

But this would be a massive refactoring that I am not planning to do on my own. If any company wants to SPONSOR this development, please get in touch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants