Fix nodes position when loading a xml file

julsam commented 5 years ago

Loading XML files didn't work. Windows only bug.

(this is not electron specific btw, the nwjs branch has the same issue)

blurymind commented 5 years ago

thank you for these pulls. I will test them out tomorrow =)

blurymind commented 5 years ago

Can you link to the issue it fixes? Like an example file. I need to test it some how :)

How do we reproduce it

blurymind commented 5 years ago

The problem with xml files it seems is bigger than just position. Some of nodes don't even load when I save to an xml file and then open it. Perhaps we should disable that feature until it's usable? What do you think

Can you provide some test xml files? I created a folder for testing files - to stress test yarn for regressions

julsam commented 5 years ago

So from what I remember when I looked into it a few months ago, it was because of this 2 lines:

avgX += object.position.x;
avgY += object.position.y;

object.position.x and object.position.y are not considered as integers but as strings. The code keeps adding them to themself, creating crazy numbers like 123456789 instead of (123+456+789)=1368

avgX and avgY are then used to warp the view to an "average" position:

if (numAvg > 0) {
     app.warpToNodeXY(avgX / numAvg, avgY / numAvg);
 }

Saving projects as xml should work just fine, it's the loading part that caused problem, but my commit should fix it.

Github doesn't allow xml files to be attached apparently, so here's an example, save it as an xml and load it using yarn:

<nodes>
    <node>
        <title>Start</title>
        <tags></tags>
        <body>Empty Text</body>
        <position x="434" y="268"></position>
        <colorID>0</colorID>
    </node>
    <node>
        <title>Node2</title>
        <tags></tags>
        <body>Empty Text</body>
        <position x="747" y="279"></position>
        <colorID>0</colorID>
    </node>
</nodes>

To reproduce the error, anything with at least 2 nodes shouldn't work without the fix.

But I don't think we should disable the feature, it should work just fine now.

blurymind commented 5 years ago

can you add to this pull request an example xml file, inside the testFiles folder :) https://github.com/InfiniteAmmoInc/Yarn/tree/electron/testFiles

julsam commented 5 years ago

Oh yeah, my bad :stuck_out_tongue: I'll add it in a few minutes.

blurymind commented 5 years ago

thank you 😄 I am going to give it a test tonight

blurymind commented 5 years ago

Merging this! Thank you for submitting a fix as well as a file to test with. It would be great if we can develop some sort of automatic file tests to catch regressions in the future

InfiniteAmmoInc / Yarn

Fix nodes position when loading a xml file #88