rapp-project / rapp-platform

RAPP Platform is a collection of ROS nodes and back-end processes that aim to deliver ready-to-use generic services to robots
Other
28 stars 20 forks source link

qr_detection wrong json documentation #348

Closed alexge233 closed 8 years ago

alexge233 commented 8 years ago

The documentation uses the below json response:

{
  "qr_centers": [ { "y": 165, "x": 165 } ],
  "qr_messages": ["rapp project qr sample"],
  "error": "..."
}

However, in the cpp API we expect:

{
  "qr_centers": [ { "y": 165, "x": 165, "message": "..." } ],
  "error": "..."
}

This has been the case since 0.5.0; the above JSON allows proper iteration, whilst the one used in the documentation makes it very hard to associate the message with the x and y assuming more than one qr_code is returned.

Please do not change the JSON response of service calls which are tested and are known to be working.

etsardou commented 8 years ago

Why is the json documentation wrong? This response was like this all along (e.g. v0.4 on 23/9/15 )

The association is quite easy and is 1-to-1, i.e. the first qr_center is associated with the first message.

alexge233 commented 8 years ago

This was not the code used in previous API's tested in 0.6.0 and 0.5.0 qrDetector.hpp and is bad form of a JSON object since it is broken inside two arrays.

Furthermore, iteration to find associated messages increases complexity and time without any advantage.

I've discussed this with @klpanagi in the past and agreed to fix it. The only change needed is:

for (var ii = 0; ii < qrCenters.length; ii++) {
     var qrPoint = { x: 0, y: 0};
     qrPoint.x = qrCenters[ii].point.x;
     qrPoint.y = qrCenters[ii].point.y;
     qrPoint.message = qrMessages[ii];
     crafted_msg.qr_centers.push(qrPoint);
}
etsardou commented 8 years ago

This is from V0.5. V0.6 is not yet published. The link you provided is from the C++ implementation and this code was created on August 15. Was this tested with the current platform?

Either way, we are in the progress of stabilizing our code, so we should not start altering the web services, the Python API and the robotic applications. Please create the C++ code with the current specification till we decide to perform a refactoring.

alexge233 commented 8 years ago

It was from the code published and tested in all previous hackathons, when I spotted the difference I told @klpanagi to fix it which I'm guessing never happened.

The current implementation as already mentioned is bad practice in both OOP and JSON format and very problematic since it can throw out of boundary exceptions.

This is a fix and not refactoring, please fix the specification and the service implementation.