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

New: Add a package type for custom plugins #715

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

Conversation

leezer3
Copy link
Owner

@leezer3 leezer3 commented Dec 16, 2021

This PR adds a package type and associated plumbing to install and load custom user plugins via the Package Install menu.

Some (minor) flaws if custom plugins are installed, which probably should be addressed at some stage:

  • No mediation between two plugins which can load the same type of file.
  • No way to enable / disable specific plugins. (Possibly wants something for all plugins as per input device plugins??)
  • No current way for a plugin to modify the icon for content it supports.

cc @zbx1425

@zbx1425
Copy link
Contributor

zbx1425 commented Dec 17, 2021

Thanks for the support!

About enabling/disabling plugins, I think route package dependency should be taken into account. For example, a route package that requires a route-loading plugin to be installed and activated to run. If the plugin can be disabled or is disabled by default (like input device plugins), this may cause some confusion.

About the API breaking changes, maybe adding a new non-abstract function, or use something like LoadProperties from IRuntime?

By the way, about the current file system configuration of package installation location, I think allowing to set different paths for routes and trains is a weird choice, and probably making the registry global is weird too, since it makes it possible for a user to create a false "dependency met" situation where the dependencies are installed but are not at the same place. For example, if a user installed package A which is a dependency of package B, then decided to change the directory due to disk space etc., and then installed package B, the dependency will be met but route in package B won't be able to find objects in package A.

@leezer3
Copy link
Owner Author

leezer3 commented Dec 18, 2021

Don't necessarily disagree with any of those in principle, but they probably come under future plans & could require route modification :)
The whole package system was conceived as a very simplistic zip extractor really, which it does well enough.

I think the next thing to do us to allow package references for objects, e.g.
{GUID}\Objects\tree.b3d
e.g.
#395

@zbx1425
Copy link
Contributor

zbx1425 commented Dec 31, 2021

I thought it over again, and I personally thought my idea is to

  1. Don't provide seperate settings for paths for Train, Railway and Other packages, just provide one setting for a base path, and install them at BasePath/Railway and BasePath/Train.
    I thought the current seperated configuration is a bit unnecessarily complex that it could be confusing to new users, as one might rarely need to install them in seperated directories.

  2. Ensure all packages are installed in the same directory, and move all files over when changing the paths.
    This tries to resolve the false dependency met scenarios.

Although I'm definitely not qualified to complain about this and that, honestly speaking, I think allowing installing them in different directories in the first place (and the decision to provide seperate settings for paths) is quite weird from the first place. I think it is too flexible than what is necessary.
Disk space might be a reason that one wants to install routes in different partitions or disks, but that would require a more elegant solution rather than the current one.

GUID referencing is nice, but do we really need that (i.e. is allowing packages to be installed here and there really necessary)

Backward compatibility is a valid concern. But keeping this as-is also has some problems, as this can effectively make the dependency system useless.

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