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

Plugin API typing #872

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

sguimmara
Copy link
Contributor

This PR exposes the supported API for the plugin system.

The root object is the TilesRendererBasePlugin, which exposes a set of optional methods. This plugin type can be extended in the derived classes, such as the TilesRendererPlugin, which adds three.js's specific API.

export class TilesRendererBase<TPlugin extends TilesRendererBasePlugin = TilesRendererBasePlugin> 

This rule conflicts with TypeScript's own typechecker in some cases,
for example the RequestInit DOM type.
Copy link
Contributor

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the contributions - as with the event types I'm hesitant to add complex typing since it already adds quite a bit of overhead to maintain. Again if this is something that Giro3D can commit to helping with then that helps to alleviate some of my concerns.

Either way, though, for the sake of simplicity I think I'd prefer to just have a single TilePlugin superclass rather than separating the three.js and subclass variant.

Also so far the plugin API have been just developed for internal use - are you guys developing custom plugins for your own use?

.eslintrc.json Show resolved Hide resolved
src/base/TilesRendererBase.d.ts Show resolved Hide resolved
@sguimmara
Copy link
Contributor Author

First of all, thanks for this awesome library. It's been a pleasure to use it and very easy to integrate in Giro3D's architecture.

Thanks again for the contributions - as with the event types I'm hesitant to add complex typing since it already adds quite a bit of overhead to maintain. Again if this is something that Giro3D can commit to helping with then that helps to alleviate some of my concerns.

We are still in the evaluation phase of integrating this library in Giro3D, but so far the results are very promising, given how modular and easy to integrate it has been. If we decide to go on and use it, then we would certainly love to contribute :)

Either way, though, for the sake of simplicity I think I'd prefer to just have a single TilePlugin superclass rather than separating the three.js and subclass variant.

The reason of the separation is to avoid referencing and importing types from three.js in the superclass. Shouldn't it remain isolated from three.js ? IIRC, the goal of this library is to be useable outside of three.js, hence the separation.

Also so far the plugin API have been just developed for internal use - are you guys developing custom plugins for your own use?

I was not aware that the plugin system was not user-facing, but it's very easy to use and helps a lot.

So far, I created a fetchData plugin to route HTTP requests to Giro3D's HTTP manager, as well as a processModel to replace materials assigned to point cloud from a .pnts tileset:

https://gitlab.com/giro3d/giro3d/-/merge_requests/792/diffs#aaf49a50063dc0fc90ffc77d4fae254b632c27a8_0_79

Later, I would also like to replace the LRU cache with our own Cache so that we can centralize and accurately track the memory usage of cached objects.

@gkjohnson
Copy link
Contributor

Glad to hear it! And thanks for sharing your use case - it's always helpful to hear.

The reason of the separation is to avoid referencing and importing types from three.js in the superclass. Shouldn't it remain isolated from three.js ? IIRC, the goal of this library is to be useable outside of three.js, hence the separation.

That is the goal - I missed that there were three.js types in the signature. Generally I've just been trying to keep the types easy to maintain but you're right it's probably best to keep them separate.

I was not aware that the plugin system was not user-facing, but it's very easy to use and helps a lot.

It's not that it's not intended to be user-facing; it's just fairly new - within the last 4 or 5 months - so it's still changing a bit, though it's starting to solidify. To some end by not documenting the exact functions I can avoid having to worry about "breaking" changes etc until it feels more complete. The goal in the long term is definitely to afford users the ability to write custom plugins, etc.

We are still in the evaluation phase of integrating this library in Giro3D, but so far the results are very promising

If these types are something you guys feel you need to evaluate the project I'm happy to merge them. The more complex the types get, though, the more overhead it become for me and the more likely they are to fall out of date, which is why I'll need help.

@sguimmara
Copy link
Contributor Author

to some end by not documenting the exact functions I can avoid having to worry about "breaking" changes etc until it feels more complete. The goal in the long term is definitely to afford users the ability to write custom plugins, etc.

Understandable. In that case, let's hold on this PR until the API stabilizes and you're confident to publish it.

@sguimmara
Copy link
Contributor Author

I can extract the eslint rule commit into a separate PR as well

@gkjohnson
Copy link
Contributor

I can extract the eslint rule commit into a separate PR as well

Sounds good to me!

@gkjohnson gkjohnson added this to the v0.x.x milestone Dec 22, 2024
@sguimmara sguimmara marked this pull request as draft December 22, 2024 08:47
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.

2 participants