Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: expose layertype #2055

Merged
merged 1 commit into from
Nov 11, 2024
Merged

fix: expose layertype #2055

merged 1 commit into from
Nov 11, 2024

Conversation

johnnyblasta
Copy link
Collaborator

Found out that this will work with plugins that has a layer type which is missing in Origo and want to add it.
The key is that the layer type should be added before starting Origo.

Closes #2031

@Grammostola
Copy link
Contributor

Would have been interesting to have partaken in your meeting as this PR is clearly for the initiated : )

It looks just like the old one.

Origo (origo, origo.js) makes bits available that plugs can utilize to add a layer type. After origo has been instantiated, reasonably.

Yet no doubt you've solved the riddle so it'll be interesting to hear how.

@johnnyblasta
Copy link
Collaborator Author

Yes, it's the same as the old PR.

The trick we missed the last time was to add the new layer type before starting Origo, since it's to late to add it when Origo already is started and the load event is dispatched which the plugin listens to get started.

This wasn't perhaps the neatest solution and we discused and tested other solutions with triggering more events in the startup before the load event, but didn't get it to work as we wanted.

And yes you probably missed an opportunity to dig down in the loading and dispatching of events in Origo.

@Grammostola
Copy link
Contributor

Grammostola commented Oct 7, 2024

Aha, as in

Origo.layerType.threeDtile = ...
const origo = Origo('index.json');
origo.on('load', function (viewer) {

Indeed, I already had another meeting (a friday afternoon :) ). Oh well, I didn't have time to answer the doodle either in time, you snooze you lose.

This seems like a realistic solution at any rate. (Edit. I think I figured it out)

@johnnyblasta
Copy link
Collaborator Author

johnnyblasta commented Oct 7, 2024

Aha, as in

Origo.layerType.threeDtile = ...
const origo = Origo('index.json');
origo.on('load', function (viewer) {

Indeed, I already had another meeting (a friday afternoon :) ). Oh well, I didn't have time to answer the doodle either in time, you snooze you lose.

This seems like a realistic solution at any rate. (Edit. I think I figured it out)

Yes, that is correct, the layertype must be assigned before kicking of Origo.

@steff-o
Copy link
Contributor

steff-o commented Oct 9, 2024

Yes, ideally we wanted a neater solution where a plugin could add itself with just one line of code. This way it is a two step operation to add the 3D layer plugin: first add the layer type before origo is initialized, then add the plugin part as usual in the load event.

Emitting more events from origo could have solved that by emitting one event just after the origo component is created but before the viewer is initialized and have the plugin listen to that event and in that handler attach to more events. Problem was that we needed to rework the start up procedure too much and that would be risky, especially if we were to accommodate for async event handlers.

We also discussed adding callbacks to origo() options, but that was scrapped for pretty much the same reasons as events plus that it wasn't as cool.

This solution, as raw it may seem, does however have some merits: it makes it possible to add functionality to the Origo-library and then use it. Not all plugins needs to add functionality, and not all added functionality needs a plugin to work.

On a personal note, I would have preferred a solution that only exposes an layertypes.add() method to avoid exposing internal implementation details. But I won't fight for it as it is just a personal preference which I can't say is better than anyone else's personal opinion.

@johnnyblasta johnnyblasta merged commit ce95747 into master Nov 11, 2024
4 checks passed
@johnnyblasta johnnyblasta deleted the exposelayertype branch November 11, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose layertype
3 participants