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

Better root path for assetlib unzipping #6023

Closed
bojidar-bg opened this issue Aug 3, 2016 · 33 comments
Closed

Better root path for assetlib unzipping #6023

bojidar-bg opened this issue Aug 3, 2016 · 33 comments

Comments

@bojidar-bg
Copy link
Contributor

Operating system or device - Godot version:
Arch Linux x64, Godot "master"

Issue description (what happened, and what was expected):
As I was installing the all-new GroupManager plugin (by @kubecz3k ), it failed to install it in the res://addons folder, but unzipped it in the root folder instead. With @akien-mga we discussed on IRC that res://addons/<name>/ would be way better for an install path for non-templates. @reduz WDYT?

Steps to reproduce:
Download any asset from asset lib, it would install it in res://, not in res://addons//

Link to minimal example project (optional but very welcome):

@akien-mga
Copy link
Member

In case the res://addons/<name> path is occupied, a unique name should be created to avoid overwriting a potential other addon with the same name. It could easily be done by appending a number to the name just like done when duplicating nodes.

@bojidar-bg
Copy link
Contributor Author

I disagree, if we do this the paths inside scenes would be totally broken.

@akien-mga
Copy link
Member

I don't see why they should be more broken than with res://addons/<name>. Do you expect people to pack their scenes in the root folder of their repo, but code them to expect paths starting with addons/<name>?
Either we fix paths ourselves on install, or we don't promise that absolute paths would work. I'm still not sure what would have hardcoded paths though, as we excluded templates already.

@kubecz3k
Copy link
Contributor

kubecz3k commented Aug 3, 2016

If you are arguing about what to do if many plugins have the same folder name... then well, when it comes to asset Store we could not allow to update plugin with exactly the same name as any other plugin that's already in a store. When it comes to manually installed plugins via Godot UI maybe just throw an informational error that plugin cant be installed, together with reason?

@reduz
Copy link
Member

reduz commented Aug 3, 2016

  1. Plugins should be always zipped from the root dir, i don't think one has
    to expect them to unzip in addons/ or anywhere in particular
  2. If files conflict, the unzip dialog already shows this and you can
    choose what to overwrite

On Wed, Aug 3, 2016 at 9:29 AM, kubecz3k notifications@github.com wrote:

If you are arguing about what to do if many plugins have the same folder
name... then well, when it comes to asset Store we could not allow to
update plugin with exactly the same name as any other plugin that's already
in a store. When it comes to manually installed plugins via Godot UI maybe
just throw an informational error than plugin cant be installed?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6023 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z207dUIr8OfTleUUrZq8W9MiPkHpuks5qcIm6gaJpZM4JbcZn
.

@akien-mga
Copy link
Member

So what's the point of the res://addons folder then? I thought it was hardcoded somewhere? (#4279)

Still, it's fully unmanageable to unzip the contents of github zip archives in res://. At the very least, 50% assets will have conflicting LICENSE.md, LICENSE.txt or README.md files.
I kind of fail to see how we want to handle all that..

@akien-mga
Copy link
Member

akien-mga commented Aug 3, 2016

Which also means that people will choose not to install the README.md or LICENSE file to avoid overwriting their own/that of another plugin, and thus miss potentially important info in the README.md, and potentially violate the license if it's one that requires to distribute the full license text and it's not hardcoded in the plugin's files.

@akien-mga
Copy link
Member

As mentioned initially, I think there's no better solution than to unzip each asset in its own folder, and let the users manage their filesystem themselves. There's nothing I dislike more than self-extracting archives that pollute my project trees.

I'd be for installing all assets to res://assetlib/<name>/ and let people do what they want with the content they download there. Some might want to keep it there for traceability, others might prefer to move the files individually to wherever they would be most sensible in their project (moving art assets to an art/ folder, or to player/john_doe/hat.png, etc.).

@bojidar-bg
Copy link
Contributor Author

I'd be for letting the user choosing his own target folder, with probably some per-asset default.

@reduz
Copy link
Member

reduz commented Aug 3, 2016

The problem is that there is a lot of things you want to download that:

  1. Are not addons
  2. Include scenes/assets, which will not like it if you move directory
  3. Include settings of engine.cfg that you might want to merge or not.

So the limitation of README.md files being polluted is unavoidable, and
trying to work around this will make the whole system a lot more complex.
If you really hate this, we could make an option to rename those files with
the name of the asset upon install, like README.assetname.md

On Wed, Aug 3, 2016 at 1:44 PM, Rémi Verschelde notifications@github.com
wrote:

As mentioned initially, I think there's no better solution than to unzip
each asset in its own folder, and let the users manage their filesystem
themselves. There's nothing I dislike more than self-extracting archives
that pollute my project trees.

I'd be for installing all assets to res://assetlib// and let people
do what they want with the content they download there. Some might want to
keep it there for traceability, others might prefer to move the files
individually to wherever they would be most sensible in their project
(moving art assets to an art/ folder, or to player/john_doe/hat.png,
etc.).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6023 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z223N5kHikhcqafWFwbKyBgcfFYU7ks5qcMWDgaJpZM4JbcZn
.

@reduz
Copy link
Member

reduz commented Aug 3, 2016

We can just harcode a list of files and document it.

On Wed, Aug 3, 2016 at 2:05 PM, Juan Linietsky reduzio@gmail.com wrote:

The problem is that there is a lot of things you want to download that:

  1. Are not addons
  2. Include scenes/assets, which will not like it if you move directory
  3. Include settings of engine.cfg that you might want to merge or not.

So the limitation of README.md files being polluted is unavoidable, and
trying to work around this will make the whole system a lot more complex.
If you really hate this, we could make an option to rename those files with
the name of the asset upon install, like README.assetname.md

On Wed, Aug 3, 2016 at 1:44 PM, Rémi Verschelde notifications@github.com
wrote:

As mentioned initially, I think there's no better solution than to unzip
each asset in its own folder, and let the users manage their filesystem
themselves. There's nothing I dislike more than self-extracting archives
that pollute my project trees.

I'd be for installing all assets to res://assetlib// and let
people do what they want with the content they download there. Some might
want to keep it there for traceability, others might prefer to move the
files individually to wherever they would be most sensible in their project
(moving art assets to an art/ folder, or to player/john_doe/hat.png,
etc.).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6023 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z223N5kHikhcqafWFwbKyBgcfFYU7ks5qcMWDgaJpZM4JbcZn
.

@reduz
Copy link
Member

reduz commented Aug 3, 2016

That won't work because assets use absolute paths, and you won't be able to
lift the asset from github if you needed to export to relative for this.

On Wed, Aug 3, 2016 at 1:57 PM, Bojidar Marinov notifications@github.com
wrote:

I'd be for letting the user choosing his own target folder, with probably
some per-asset default.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6023 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z29KgXuTD45QYHFv5C1KhMb9kmsEBks5qcMiSgaJpZM4JbcZn
.

@eon-s
Copy link
Contributor

eon-s commented Aug 4, 2016

What about a "res://assetlib" directory created with the project and set as default assetlib install location? (or add an option to set the default location when creating a project)
If a tool need a specific path, that should be on the readme and can be fixed by the user after installing it.

ps: Also, give the option to create a folder (or two) when installing, I would like to put my downloaded tools in assetlib/tools, scripts on assetlib/scripts, etc., if possible.

@reduz
Copy link
Member

reduz commented Aug 4, 2016

nope, Godot uses absolute paths so it's simply not possible to unzip into a
different location than res://

On Wed, Aug 3, 2016 at 10:39 PM, eon-s notifications@github.com wrote:

What about a "res://assetlib" directory created with the project and set
as default assetlib install location? (or add an option to set the default
location when creating a project)
If a tool need a specific path, that should be on the readme and can be
fixed by the user after installing it.

ps: Also, give the option to create a folder (or two) when installing, I
would like to put my downloaded tools in assetlib/tools, scripts on
assetlib/scripts, etc., if possible.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6023 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z20_P6Pj6GHL7Uzm2JobLN3lMFrEQks5qcULngaJpZM4JbcZn
.

@reduz
Copy link
Member

reduz commented Aug 4, 2016

You can do this in Unity because files are referenced by a metadata ID, but
we decided not to do this in Godot for the sake of simplicity.
As a result, assets go to res:// and nowhere else.

On Wed, Aug 3, 2016 at 10:52 PM, Juan Linietsky reduzio@gmail.com wrote:

nope, Godot uses absolute paths so it's simply not possible to unzip into
a different location than res://

On Wed, Aug 3, 2016 at 10:39 PM, eon-s notifications@github.com wrote:

What about a "res://assetlib" directory created with the project and set
as default assetlib install location? (or add an option to set the default
location when creating a project)
If a tool need a specific path, that should be on the readme and can be
fixed by the user after installing it.

ps: Also, give the option to create a folder (or two) when installing, I
would like to put my downloaded tools in assetlib/tools, scripts on
assetlib/scripts, etc., if possible.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6023 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z20_P6Pj6GHL7Uzm2JobLN3lMFrEQks5qcULngaJpZM4JbcZn
.

@bojidar-bg
Copy link
Contributor Author

@reduz What about doing dependency fixation after/while the unzip happens?

@eon-s
Copy link
Contributor

eon-s commented Aug 4, 2016

@reduz I can't argue against simplicity.
So, assetlib content creators may need to follow some (informal) protocol like using reverse domain name notation for the main project directories and detailed readme, license.

@reduz
Copy link
Member

reduz commented Aug 4, 2016

Yeah, I prefer we fix this with docs than code

On Aug 4, 2016 3:28 PM, "eon-s" notifications@github.com wrote:

@reduz https://github.com/reduz I can't argue against simplicity.
So, assetlib content creators may need to follow some (informal) protocol
like using reverse domain name notation for the main project directories
and detailed readme, license.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6023 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2-TXUn_ZZojHelWly0SuQbDB06IDks5qci9kgaJpZM4JbcZn
.

@Zylann
Copy link
Contributor

Zylann commented Aug 5, 2016

So for example, my terrain plugin would be installed at:
res://assetlib/zylann/terrain/plugin.cfg ?
Which is res://<non_user_created_folder>/<maintainer_name>/<plugin_name>/.

By the way, this issue is about filesystem namespace, but there will be a similar issue with types created by plugins (we don't have any namespace system). What if two plugins have a node name conflict? (but I guess it will become a separate issue in the future)

@eon-s
Copy link
Contributor

eon-s commented Aug 6, 2016

@Zylann I think it will be just:
res://<maintainer_project>

As it is now, the asset creator has to create the all the folders and files as she/he wants it to appear in the user project.

So if you want it:
res://assetlib/zylann/terrain/
You need to make a project :
/assetlib/zylann/terrain/

For plugin problems, maybe that should be managed by the plugin enabler, showing a message if there is a conflict with an active plugin (I don't know if that is possible or already exists).

@bojidar-bg
Copy link
Contributor Author

Here is another option, though it is pretty hard to implement properly -- have each addon have its own res:// scope, unrelated to other addons. The only way to reference non-scope resources then would be by exporting them.

Though that would take a week to implement properly, I'm listing it for completeness 😄

@Zylann
Copy link
Contributor

Zylann commented Aug 6, 2016

@eon-s the reason why I used assetlib (or whatever) as root is to make it more official that "this part of the tree is third-party". It's a bit of a personal preference, but at least it won't pollute the root folder namespace with folders from a different origin/meaning.

@bojidar-bg I kinda like the idea but the fact other addons are inaccessible might be problematic if your addon can extend/communicate another. What do you mean by "exporting them"?

@reduz
Copy link
Member

reduz commented Aug 6, 2016

I still think you guys are worrying too much about something that is not
really a problem. We should simply define some sort of standard naming
convention for projects and that's it

On Sat, Aug 6, 2016 at 7:21 AM, Bojidar Marinov notifications@github.com
wrote:

Here is another option, though it is pretty hard to implement properly --
have each addon have its own res:// scope, unrelated to other addons. The
only way to reference non-scope resources then would be by exporting them.

Though that would take a week to implement properly, I'm listing it for
completeness 😄


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6023 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2wObFdm73TK09OTnm0Jdv_Gz58K3ks5qdGArgaJpZM4JbcZn
.

@bojidar-bg
Copy link
Contributor Author

@reduz I agree, I was just listing it as a yet another solution

@vnen
Copy link
Member

vnen commented Aug 6, 2016

Currently plugins are looked into addons/plugin right? So maybe it's simpler to just make the plugin folder have some namespacing convention like addons/vnen.myplugin.

@reduz
Copy link
Member

reduz commented Aug 6, 2016

I wonder if we have more chances of the plugin name colliding than the
username colliding :P

On Sat, Aug 6, 2016 at 7:10 PM, George Marques notifications@github.com
wrote:

Currently plugins are looked into addons/plugin right? So maybe it's
simpler to just make the plugin folder have some namespacing convention
like addons/vnen.myplugin.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6023 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2wcowYAp6Yq2V59PyAAiAv622q7Xks5qdQZogaJpZM4JbcZn
.

@Zylann
Copy link
Contributor

Zylann commented Aug 9, 2016

Godot 2.1 is out, but my plugin doesn't works because my .cfg is in addons/zylann/terrain/plugin.cfg, and Godot doesn't looks that far. I can hack engine.cfg to it looks to zylann/terrain instead of terrain, but it doesn't makes the plugin visible in the settings. Should I make it so pluging.cfg is one folder above so Godot finds it? Or will plugin.cfg files be found recursively in a next release?

@bojidar-bg
Copy link
Contributor Author

@Zylann Can you do addons/zylann.terrain? Like @vnen suggested.

@Zylann
Copy link
Contributor

Zylann commented Aug 10, 2016

@bojidar-bg I don't like prefixing very much, given that it could be as simple as a folder in the filesystem. I can do that in the meantime but I'd like to know if it's going to stay that way?

@bojidar-bg
Copy link
Contributor Author

@Zylann That's sole at @reduz's descision I think.

@Zylann
Copy link
Contributor

Zylann commented Aug 14, 2016

IMO the editor should be able to spot plugins that are a bit deeper. Actually, it already works, you can write enabled = ["zylann/terrain"] in engine.cfg. But for some reason the editor won't show it :/

@BastiaanOlij
Copy link
Contributor

I haven't read everyones comments however one thing that is important is that not all things in the asset library are actually add ons. Also changing paths can break the add on because many paths are set in the scene files.

I would suggest adding a path to the meta data in the asset library itself so the person releasing the asset still has control. So in the asset library I have defined to install my add on in 'addons/myaddon' and that is where Godot downloads the repository files too.

Note that depending on the type of asset you're registering in the asset library the asset library could suggest what that folder name should be. So for a demo asset, it will just be the root (empty), while for an addon it will suggest 'addons/myaddon'

@Calinou
Copy link
Member

Calinou commented May 26, 2020

Closing in favor of godotengine/godot-proposals#554, as feature proposals are now tracked in the Godot proposals repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants