pixi-essentials icon indicating copy to clipboard operation
pixi-essentials copied to clipboard

Please add original svg element's IDs as name to children so we can use getChildByName()! (see code)

Open bencresty opened this issue 2 years ago • 8 comments

First of all; thanks a lot for this great project! It's so nice to be able to use SVG inside pixi while keeping it sharp while zooming!

I want to change properties of a child inside the SVG Scene, like its fillColor. I found out how to do that, but having a huge SVG how can we find the right child by its ID as set in the SVG Elements ID attribute? I've searched through the properties of for example the SVGPathNode class, but so far I couldn't find anything that tells the actual ID/name of the child.

So my question is: where is the ID stored inside a child and/or how to find a child by it's original element's ID?

Thanks in advance!

bencresty avatar Feb 03 '22 15:02 bencresty

Looking further into the code I can't find any reference to the original svg elements by their ID or class.

When adding node.name = element.id; on top of the populateSceneRecursive() method this seems to be easily fixed in seconds:

When adding this in svg.es.js from line 3643 this makes us able to find elements by their name:

populateSceneRecursive(
        element,
        options

,
    )
    {
         const node = this.createNode(element);
         node.name = element.id;
         
         // ...

We can then just use the built in Pixi way to query elements from the SVG Scene like this:

const test = this.svgScene.root.getChildByName('myObjectsId');

It's one line of extra code that makes all the difference in navigating the SVG Scene!

Any chance this could be implemented?

@pixi-essentials/svg version: 1.1.5

bencresty avatar Feb 03 '22 17:02 bencresty

@bencresty The way I recommend you do it is override populateSceneRecursive and store the node created and associate it with the SVG element in a map :)

ShukantPal avatar Feb 04 '22 00:02 ShukantPal

@bencresty The way I recommend you do it is override populateSceneRecursive and store the node created and associate it with the SVG element in a map :)

Thanks for your response.

When working with a scene graph the first thing I would expect to work and use is to be able to query its children. This is the way Pixi works, but also other scene graphs. Like in threeJs, SVG, the DOM and other scene graphs libs. Knowing this 'lib' is build for pixi I am surprised it's not using the pixi way to do this.

Is there anything against making this work the pixi-way by just adding that one line as mentioned?

bencresty avatar Feb 04 '22 07:02 bencresty

Is there anything against making this work the pixi-way by just adding that one line as mentioned?

No, not really. Feel free to make a PR. I can publish to npm :)

ShukantPal avatar Feb 04 '22 14:02 ShukantPal

@ShukantPal If you're going to push this to npm, and you clearly have the project up and running on your system, why ask me to set everything up just to add that one single line, where I even gave you the exact location of as line number and I tested it for you? Isn't it easier and more efficient you just copy'n'paste that line I wrote for you above into your current state of the project code and publish it?

bencresty avatar Feb 04 '22 15:02 bencresty

lol With the time you spent whining, the PR could've been made by now. /s

You can press "." on the GitHub repo to open VS Code in the browser -- no need to setup anything. And then I can run a script to publish to npm without doing any work.

ShukantPal avatar Feb 04 '22 15:02 ShukantPal

@bencresty What is supposed to happen if the SVG contains duplicate IDs?

mootari avatar Nov 17 '22 10:11 mootari

@mootari duplicate ID's in SVG is illegal so may never happen. So there doesn't need to be a renaming when this happens. Just always copy the ID string to use as name. No matter if it's a duplicate or isn't. It doesn't mess things up, it's only a name string and to my best knowledge Pixi names don't need to be unique.

The only thing the getChildByName() needs to do is to return the first child it finds by the given name.

bencresty avatar Nov 18 '22 17:11 bencresty