Missing call to replaceParent in group.js?

[dulimarta]

Describe the bug
Two.shape objects are not inserted correctly to the desired group when done from a mouse handling function. Instead, they are inserted as an immediate child of the <svg> node.

To Reproduce
I'm not sure how to describe the simplified steps to reproduce the bug by trimming down our VueJS project which currently has 350+ files. But I believe the bug shows up whenever a new Two.Shape is added to a group from a mouse event handling.

My attempt to fix
So, instead of describing how to reproduce the bug, I will describe the changes I made to group.js that seems to fix the issue for me. I found this fix after debugging the add() function in group.js and noticed that the replaceParent() is not always called.

The following two functions add() and remove() are in group.js (version 0.8.9)

add(objects) {
    // original code not show
    for (let i = 0; i < objects.length; i++) {
        const child = objects[i];
        // original code not shown
        this.children.push(child);
        replaceParent(child, this);     // <-- I inserted this line at the end of the for-loop
    }
   return this;
}

remove(objects) {
    // original code not show
    for (let i = 0; i < objects.length; i++) {
        const object = objects[i];
        // original code not shown
        if (index >= 0) {
            this.children.splice(index, 1);
            replaceParent(object);     // <-- I inserted this line at the end of the for-loop
        }
    }
   return this;
}

Expected behavior
Objects are inserted into the desired group. Adding the call to replaceParent() fixes the issue for me.

Screenshots

Environment (please select one):

• Code executes in browser (e.g: using script tag to load library)
• Packaged software (e.g: ES6 imports, react, angular, vue.js)
• Running headless (usually Node.js)

Desktop (please complete the following information):

• OS: Windows 11, OSX
• Browser: FireFox 101.0

[jonobr1]

I'll take a look, but this bug could be related to how you mix and matched imports in this bug

[dulimarta]

I'll take a look, but this bug could be related to how you mix and matched imports in [](this bug)

I was able to workaround that issue by creating a "detached" group (i.e. not attached to the scene) by using the following code (more or less):

const twoInstance = new Two({/* options go here */});
const dummy_group = new Group(); // detached group to ignore duplicate two-0

for (let k = 0; k < N; k++) {
  const g = new Group();
  g.addTo(twoInstance.scene);
}

[jonobr1]

This simple demo seems to work fine: https://codepen.io/jonobr1/pen/VwQdNjx?editors=0010

[dulimarta]

This simple demo seems to work fine: https://codepen.io/jonobr1/pen/VwQdNjx?editors=0010

Thank you for confirming that it works with your example.

To understand how TwoJS Group works on my actual project, I added console,.debug() inside replaceParent() to confirm that the function is indeed called, but on the contrary I noticed that the function is not always called and hence the extra replaceParent() call I added in both add() and remove(). Reading into group.js I understand how the static functions (Insert|Remove)Children are bound for handling Events.Types.(insert|remove), however I don't quite understand how the event mechanisms are used in TwoJS.

Also, while debugging the issues in my project, I also noticed that adding replaceParent in remove() is not sufficient. I also learned that the for-loop skips some objects (with continue) when they are not found in children.ids while in fact the parent of these objects is indeed the correct one (at least as observed from console.debug() output), i.e. I found cases when X's parent is P but P's children.ids does not record X as a child. The only way to make this works specifically for my project, I had to comment out the if....{ continue } lines in remove()

To compare my biggish project with your example, I also wrote an example similar to yours using Parcel + TypeScript and I confirmed that on this simple example, replaceParent() is always invoked (using the untouched copy of add() and remove(). This leads me to believe that in my project Events.Types.(insert|remove) are suppressed or blocked, or not triggered.

So, if you could briefly explain how these events are used by TwoJS, it would help me a lot in debugging the real bugs that are hidden in (other parts of) my project.

[jonobr1]

Makes sense. The replaceParent logic is admittedly confusing and a portion of the code I didn't originally write. The way that it works though is that Two.Group.children is an instance of Two.Collection. Two.Collection is an array with added event triggers when you call methods like push, splice, concat, etc.. When you do this the collection emits an event. Sometimes it's InsertChildren sometimes it's RemoveChildren and sometimes it's both.

Perhaps if you're calling a lot of operations at once, this event emission gets misinterpreted and the order gets screwed up. For example, I'm thinking some commands like this:

var group = new Two.Group();
group.push(a, b);
group.splice(1, 1);
group.shift();
group.push(b);

Also, Two.Group.add and Two.Group.remove invoke these functions under the hood.

The successive calls could screw with what should be operated on because b in the example above needs to be added, removed, and then added back in. I need to sit down and test this, but maybe Two.js tries to add once (cause it's already been flagged to be inserted) and then gets removed and so the last 'push' is left out.

Hope this helps explain things. It certainly highlights an inconsistency I've been blind to.

[dulimarta]

Ah, thank you so much for explaining the inner working of the TwoJS events. After reading your explanation, I start to have a guess why those TwoJS events are not triggered in our project. Earlier in this conversation thread, I mentioned that our project uses VueJS. By the way this project is a collaboration between @dickinson0718 and me. I included his name because he recently submitted the following issues/enhancement requests:

• #605: Two.Arc enhancement
• #634: Omissions in type.d.ts and
• #636: TS migration

We are grateful that you have been very responsive in our communications.

Since VueJS also overrides the behavior of the array functions: push(), pop(), splice(), splice(), and many others, I suspect these events are not passed on to TwoJS and hence the bugs that show up specifically in our VueJS-based project but not under a non-VueJS sample that you and I both tested and observed.

I'd like to hear your idea for making this work under our VueJS project (assuming my assumption about the events not being triggered).

Also, I'd like hear your input on my temporary solution below:

1. Manually calling replaceParent() in add() and remove()
2. Commenting out the children.ids membership check in remove()

whether these modifications may have negative impact on the other TwoJS functionalities.

[jonobr1]

Hmm, interesting. I need to look into how Vue.js alters an Array, but if you look at Two.Collection's source code (link), you'll see that it inherits from an Array and overrides most of those methods. When needing to fire events Two.js relies exclusively on Two.Collection, so I'm not sure that any Array modifications would actually affect Two.Collection in the way you're describing.

Here's a question for you, are you setting and/or modifying the children property?

[dulimarta]

Here's a question for you, are you setting and/or modifying the children property?

No, my code does not directly modify the children property, we always use addTo() or remove() to insert/remove our TwoJS objects to a group.

[jonobr1]

Is there anyway to make a version of your code that reproduces the problem on codesandbox or something? It's pretty difficult to triage this from snippets.

In general, I don't think it's a problem to always run replaceParent, but we have extensive tests in place to make sure edge cases are / aren't caught. Making those modifications does break the tests (as seen below). But, since the events don't bubble, perhaps this isn't an issue.

[dulimarta]

I wish I could easily scale down our app to reproduce the issue and post the scaled down version online. However, it has too many moving parts (VueJS 2.x, Vuetify 2.x, Pinia, and TwoJS itself). We did not see this issue when using TwoJS 0.7, but only after upgrading to 0.8. The main reason upgrading to 0.8 is the new Two.Arc that will simplify many of curve rendering logic in our app. So, I would think that some design changes between TwoJS 0.7 and 0.8 that prevent the update events to propagate upwards.

[jonobr1]

Thanks again for your commitment. I went through and totally rewrote the Two.Group.add / Two.Group.remove logic here. It's not performant (yet), but any chance you can copy it into your project and give it a try? Let me know if it fixes any / all of the addition problems.

Replace your npm install with npm i --save https://github.com/jonobr1/two.js#issue-640-replace-parent

[dulimarta]

Thank you for your extra effort of rewriting add() and remove() of Two.Group. I tried the above TwoJS patch you mentioned, but it did not seem to fix the issue; the SVG children (Two.Path) were not attached under the intended group. Essentially, it shows the same SVG structure as shows in the screenshot of Jun 3 at the beginning of this issue.

[jonobr1]

🤔 any chance you could give me access to the project then? Where I could reproduce it locally on my end?

At this point, I'm just really curious.

[dulimarta]

Yes. of course.

Our source code is on GitHub: https://github.com/dulimarta/spherical-easel. In particular there is branch twojs_update when @dickinson0718 and I upgraded our app from TwoJS 0.7 to 0.8.

In commit 251931 we are using TwoJS 0.8.10.
The simplest scenario to try: activate the Create Point tool and place the mouse cursor in the first quadrant (sphere center is assumed (0,0). Using TwoJS 0.8.10 you will see that the rendered point is off about half of the circle width on the X-axis and moves in the opposite Y-direction from the mouse cursor. This is because our app applies CSS matrix transformation to move the sphere center and flip the Y-axis to the Two.Group where the point is supposed to be inserted, but the point is not actually inserted to the correct Two.Group.

When I use my own TwoJS Fork, the app works as expected.

[jonobr1]

Thanks for sharing.

I'm using Node.js v17.7.2 and yarn serve fails with the error "Digital envelope routines::unsupported" around cryptohash: this[kHandle] = new _Hash(algorithm, xofLen);. Any ideas?

[dulimarta]

Never saw that error before. I use NodeJS 16.13.2 and NPM 8.1.2.

Also instead of yarn serve (which launches both the VueJS and VuePress local servers), I use yarn app:serve to launch only the VueJS local server. It is not related to the error you cited above, but it makes launching a bit faster.

[jonobr1]

Small update

Some things I noticed while trying to debug:

• You don't need autostart if you're going to call two.play(); later: https://github.com/dulimarta/spherical-easel/blob/main/src/components/SphereFrame.vue#L142

I think I was able to find the problem. For some reason the way you're declaring and setting layers is problematic. If you comment out private layers: Two.Group[] = []; and then replace this.layers.splice(0, this.layers.length); with this.layers = []; it works fine for me. I think it has something to do with all the decorators you're using, it's honestly incredibly difficult to read and I wouldn't be surprised if you have a lot more consistency issues than just the one outlined in this thread.

I'm going to try to keep poking around to understand why it is like that.

[dulimarta]

Thank you for spending time debugging our code and looking into the source code and giving us feedback. After following the link and reading your response above where you referred to specific statements, I assume that you are reading/debugging our code from the main branch where we still use TwoJS 0.7.x. As mentioned in my message on Jul 12, the problematic source code is under the twojs_update branch.

[jonobr1]

I was using the main branch, but was able to reproduce the issue simply by updating Two.js to 0.8.10