Canvas reference

[tothatt81]

Let me quickly try and summarise the issue.

According to the documentation, there are currently 2 ways of instantiating a Viewer:

1. Connect an existing canvas DOM element with the sdk by passing its ID to the Viewer with the canvasId config option.
2. If no ID is given, the sdk will create a new full page canvas to use.

Generally speaking it's a bit easier to pass merely an ID rather than the reference of an existing DOM element. In modern JS frameworks, however, such as React or Vue, it’s trivial to grab a reference to an element without having to specify an ID.

For instance, take the xeokit-react library. I think that it should particulary benefit from an ID-less solution, as they would be able to provide their users a cleaner API.

Therefore, I propose that the sdk be extended to accept an ID-less configuration as well. I've come up with 3 ideas to this end:

1. Instead of the current canvasId config option, there could be a new one named canvas. This would be the cleanest approach but is also a breaking change. Anyone using the canvasId option (pretty much everyone right now) would get a full-page canvas which may not be desirable. Since the enhancement I’m suggesting here isn’t exactly of high priority, I’d imagine that you wouldn’t want to introduce any breaking changes. You’d also have to rewrite all the examples to use the new config.
2. We could keep canvasId but we’d also accept the canvas element itself. This comes with its very own advantages and disadvantages. On the one hand, it isn’t a breaking change, which is nice, obviously. On the other hand, the name canvasId would no longer be very accurate and semantic if it accepts not just an ID but a canvas element as well.
3. We would keep canvasId. But we would also add another config option, let’s say canvasElement. In the latter case, I could give it the DOM element itself. One drawback might be that the user could potentially pass both the ID and the element which doesn’t make a whole lot of sense at the end of the day. However, in such a case, we could simply prefer one of these to the other. We’d only need to clearly state in the documentation that when both values are given, the element reference is always preferred to the ID. Other than that, if only an ID is given, that’s the one that’ll be used, if only an element is given then that’s what'll be used. Fairly simple. If none of them, then we revert to the full-page canvas as it is now.

To sum up, the 3rd option might be the best one in my opinion.

If you think this would be a worthwhile enhancement, please let me know and I'd be happy to take a stab at it and submit a PR.

Cheers,

tothatt

[barnabasmolnar]

This is a very timely post from @tothatt81 as I'm currently in the process of conducting some research into how I could refactor the xeokit-react library into a completely headless and potentially react hooks based component.

The aim would be to provide a clearly defined list of useful methods and props that integrate many of the xeokit-sdk's excellent features while being fully decoupled from the presentational layer. The increased modularity and separation of concerns would be quite advantageous from a number of perspectives. Anyway, I'll try and write it up over at the xeokit-react github page in the near future.

Here, I just wish to say that whilst probably not strictly necessary for us from a purely technical standpoint, tothatt's proposed solution would in fact make our lives a fair bit easier during the refactor process. :+1:

[xeolabs]

Thanks @tothatt81, I think idea #3 is the way to go, so yes a PR would be most welcome. BTW can I get you to sign the contributor's agreement first: https://github.com/xeokit/xeokit-sdk/wiki/Contributor-License-Agreement

[barnabasmolnar]

@tothatt81 @xeolabs Hey guys, just a heads up for ya

I've been looking at the source code for the Viewer and Scene modules and I've noticed an inconsistency between what the docs say and what actually happens.

As tothatt said in his post:

If no ID is given, the sdk will create a new full page canvas to use.

Which is what the docs indicate here: https://xeokit.github.io/xeokit-sdk/docs/class/src/viewer/Viewer.js~Viewer.html#instance-constructor-constructor

cfg.canvasId | String | optional | ID of existing HTML canvas for the Viewer#scene - creates a full-page canvas automatically if this is omitted

This is however in contrast with the actual source code here: https://github.com/xeokit/xeokit-sdk/blob/master/src/viewer/scene/scene/Scene.js#L335

if (!cfg.canvasId) {
    throw "Mandatory config expected: canvasId";
}

@tothatt81, any suggestions on how to deal with this?

I was thinking that since you're adding the canvasElement option, technically both that and canvasId would be optional. However one of them would have to be passed in. Perhaps change the wording of the corresponding ESDoc comments to reflect this change?

[tothatt81]

Hey Barna,

Thanks for the heads up, very useful!

Yes, I'd be happy to update the documentation too. In fact, there's one more thing I've noticed. It concerns the NavCubePlugin. That expects an ID too but while I'm already at it, I could expand that to accept an element as well.

I'd also implement some error handling improvements. I hope that would be fine, @xeolabs.

Cheers,

Attila

[xeolabs]

@tothatt81 - good catch, updating the documentation, NavCubePlugin and error handling would be fine with me!