Open dulimarta opened 2 years ago
I'll take a look, but this bug could be related to how you mix and matched imports in this bug
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);
}
This simple demo seems to work fine: https://codepen.io/jonobr1/pen/VwQdNjx?editors=0010
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.
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.
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:
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:
replaceParent()
in add()
and remove()
children.ids
membership check in remove()
whether these modifications may have negative impact on the other TwoJS functionalities.
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?
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.
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.
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.
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
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.
🤔 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.
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.
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?
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.
Small update
Some things I noticed while trying to debug:
autostart
if you're going to call two.play();
later: https://github.com/dulimarta/spherical-easel/blob/main/src/components/SphereFrame.vue#L142I 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.
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.
I was using the main
branch, but was able to reproduce the issue simply by updating Two.js to 0.8.10
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 theadd()
function ingroup.js
and noticed that thereplaceParent()
is not always called.The following two functions
add()
andremove()
are ingroup.js
(version 0.8.9)Expected behavior Objects are inserted into the desired group. Adding the call to
replaceParent()
fixes the issue for me.Screenshots
Environment (please select one):
Desktop (please complete the following information):