search

open-rmf / rmf_site

Experimental visualizer for dense buildings in RMF
32 stars 13 forks source link

Make modules in librmf_site_editor public #161

Closed Yadunund closed 1 year ago

Yadunund commented 1 year ago

To enable downstream applications to run rmf_site_editor with custom plugins.

mxgrey commented 1 year ago

@Yadunund I've made a little tweak, which is to not make fn init_settings public.

I assume for your use case you've probably been manually rewriting this chunk of code in your downstream application. As of #162 you can now simply do app.add_plugin(SiteEditor) so there shouldn't be a need for init_settings to be public.

Generally speaking, making all these modules public is kind of a risky thing to merge. Any symbols (functions or structs) that are public to downstream users imply that we are committed to keep their APIs stable. Realistically that is not the case, since this whole repo is under rapid and massive development.

I think before we publish a true release of the crates in this repo, we need to do an audit of what things should really be public versus what should be kept private. Essentially, what do we really consider the inputs/outputs of the site editor to be, and what do we believe the relevant "hooks" are for downstream applications.

But since there are downstream projects actively experimenting with site editor, I am okay with cracking these open for now and seeing how the projects evolve. This message is just a word of caution to experimenters that we should all expect the public API to change and tighten up before a true release happens.

Yadunund commented 1 year ago

I assume for your use case you've probably been manually rewriting this chunk of code in your downstream application. As of https://github.com/open-rmf/rmf_site/pull/162 you can now simply do app.add_plugin(SiteEditor) so there shouldn't be a need for init_settings to be public.

That's right. One reason why I can simply do app.add_plugin(SiteEditor) is that I have a custom MainMenuPlugin and do not want to use the one from here. So I'm afraid I require init_settings to be public. Else I can redefine it in my pkg.

mxgrey commented 1 year ago

I see, that's a valuable data point which tells me we should offer a more granular breakdown of the SiteEditor plugin, so people can choose what aspects of the site editor application are included downstream.

I've pushed a new commit that makes init_settings public again. I think it's better to not reimplement that since we might start to support user settings files in the future which might be read by that system.

Yadunund commented 1 year ago

Thanks!