viniciusgerevini / godot-aseprite-wizard

Godot Editor plugin to help import Aseprite animations to AnimationPlayers, AnimatedSprites and SpriteFrames.
MIT License
891 stars 42 forks source link

Store config in metadata #63

Closed dfkeenan closed 1 year ago

dfkeenan commented 2 years ago

This PR is for #61

I have made changes so it stores the config in object metadata instead of using the editor_description property.

It can still load config from the editor_description property if it exists. But if import is run again it will store the config in metadata and clear the editor_description property, but only if it is containing aseprite-wizard config.

As I previously mentioned in issue #61 this data does get included in the exported game. I did workout how to make it so it could remove this data on export but have not included it yet because I think there should be some discussion bout this. Some talking points:

viniciusgerevini commented 2 years ago

Thank you. Let me know when it's ready to be tested and I can run some tests on my side.

About the EditorExportPlugin. I think this is optional. You mentioned in the issue that the editor_description is already included in the scene meta, so there is virtually no change on what is shipped. Also, the meta size is probably less than adding a node to the scene.

If you see it as a good improvement, we can totally have it as an extra feature toggled off by default. Once we prove it does not conflict with other plugins we can move it to on by default. But if this increases too much the complexity, I would leave it out. At least for the first version.

dfkeenan commented 2 years ago

The reason I looked into the EditorExportPlugin was because I noticed that if you use an aseprite file that is outside the project folder the path used is the fully qualified path. Could possibly expose personal data. Not including the image in the project probably not that common. At least if you putting your project in source control. Probably a low risk.

If you are happy with what I have done so far I could change PR to ready for review. We could leave the EditorExportPlugin for a later PR if you like.

viniciusgerevini commented 2 years ago

Could possibly expose personal data

this is a really good point. In this case, the cleanup seems a big part of it and we shouldn't have one without the other. Maybe the whole feature should be behind a settings option for the first version.

dfkeenan commented 2 years ago

Something like this for metadata option?

image

What about the option for turning the export option on/off. Should that be in it's own tab called "Export"?

dfkeenan commented 2 years ago

I added the EditorExportPlugin with an option to enable it. Not sure about that name. Currently you have to reload the project for that setting to take effect. Which seems to be the same behavior as the enable importer plugin setting.

image

dfkeenan commented 2 years ago

Not sure about the EditorExportPlugin implementation. I tested it. It does what I want it to do (remove metadata). Not sure if I did it the best way.

If you are generally ok with it I can change to ready for review.

viniciusgerevini commented 2 years ago

Hello @dfkeenan . This is looking great.

About the export plugin config. Given the proximity/dependency with the Use metadata option, I would leave it under same category ("storage"). One suggestion for the config name is Enable metadata removal on export, but honestly this is subjective and I'm fine if you prefer the way it is.

I'll find some time to run some extra tests on my side this week and I think it will be good to go. Thank you!

dfkeenan commented 2 years ago

I don't have a problem with changing location and name of that config setting. That does sound better. I probably won't get to it until next weekend.

dfkeenan commented 2 years ago

I have made the suggested config change. I think the PR is ready for review.

viniciusgerevini commented 1 year ago

Thanks again. It works great. I'll merge this and release in the next version. 🚀