search

KanoComputing / kano-code

👩‍💻👨‍💻Kano Code
https://world.kano.me
GNU General Public License v2.0
16 stars 5 forks source link

Only round translation values at render #1826

Closed keithclark closed 4 years ago

keithclark commented 4 years ago

This PR fixes a rounding bug with transform API when moving elements with moveAlong function (called by the "move distance" block).

The moveAlong function is rounding transform values each time it's called. This introduces calculation errors when the method is called as part of a chain of transform functions. This change moves the Math.round call to the render method so rounding only occurs when the transform is used to draw.

TeamKano commented 4 years ago
Coverage summary
Statements 27.86% 1900/6820
Branches 19.23% 199/1035
Functions 27.63% 610/2208
Lines 27.71% 1834/6618

From Jenkins: kano-code/PR-1826

rachaelcodes commented 4 years ago

Hi Keith, thanks for bringing this to our attention. We were wondering, do you have an example of some code where this is causing a problem? Thanks

keithclark commented 4 years ago

Hi Rachael. Here's a simple app that shows the problem. I'm loading it into the editor directly via devtools with Kano.Code.mainEditor.load():

{
  "source": "<xml xmlns=\"http://www.w3.org/1999/xhtml\"><block type=\"app_onStart\" id=\"default_app_onStart\" x=\"118\" y=\"91\"><field name=\"FLASH\"></field><statement name=\"CALLBACK\"><block type=\"expected_moveTo\" id=\"zYKrs4vR52+@_e-7g7q|\"><value name=\"X\"><shadow type=\"math_number\" id=\"ClFP`d;5|j4q0Vwg/v9j\"><field name=\"NUM\">626</field></shadow></value><value name=\"Y\"><shadow type=\"math_number\" id=\"x2L7/OF~y/T,6sGpf%OG\"><field name=\"NUM\">405</field></shadow></value><next><block type=\"actual_turn\" id=\"SW4a%1/^VhOpnL=W-d|W\"><field name=\"TYPE\">clockwise</field><value name=\"ROTATION\"><shadow type=\"angle\" id=\"u;RZ@/GC$Cf5Xq=P3bRC\"><field name=\"VALUE\">25</field></shadow></value><next><block type=\"repeat_x_times\" id=\"TDW78lk0w;1K9+yFi)85\"><value name=\"N\"><shadow type=\"math_number\" id=\"qS~mtrLQ$BG|Ox}=_/kv\"><field name=\"NUM\">250</field></shadow></value><statement name=\"DO\"><block type=\"actual_moveAlong\" id=\"7n*Q=iSJ0/c;N?ViJj/2\"><value name=\"DISTANCE\"><shadow type=\"math_number\" id=\"WiK8^,r5NcBfro`FM`Z]\"><field name=\"NUM\">1</field></shadow></value></block></statement><next><block type=\"actual_turn\" id=\"up~QNz!RzD0cnm;Sdssp\"><field name=\"TYPE\">counterclockwise</field><value name=\"ROTATION\"><shadow type=\"angle\" id=\")*b4Cil=OvCYMlBZ]w/4\"><field name=\"VALUE\">25</field></shadow></value></block></next></block></next></block></next></block></statement></block></xml>",
  "code": "app.onStart(function() {\n  expected.moveTo(626, 405);\n  actual.turnCW(25);\n  for (var i = 0; i < 250; i++) {\n    actual.moveAlong(1);\n  }\n  actual.turnCCW(25);\n\n});\n",
  "parts": [
    {
      "type": "sticker",
      "id": "expected",
      "name": "Expected"
    },
    {
      "type": "sticker",
      "id": "actual",
      "name": "Actual"
    }
  ],
  "profile": "default"
}

The app contains two stamps, "Actual" and "Expected". Actual is rotated 25 degrees CW, moved 250 pixels (in 1 pixel increments to demonstrate the bug) and then rotated 25 degrees CCW. The Expected stamp shows where Actual should end up.

The two stamps should be drawn on top of each other (+/- a couple of pixels) but, as the screen grab below shows, that isn't the case. On master (pictured on the left), despite being rotated to 25 degrees, the stamp doesn't move along the Y axis because of premature rounding (it actually snaps to 45 degree intervals). If you run the app in this branch (pictured on the right), the stamp moves as expected.

results