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

Revert palette file format back to JSON #967

Merged
merged 23 commits into from
Dec 22, 2023

Conversation

OverloadedOrama
Copy link
Member

This PR is reverting the palette file format back to json, as it used to be some versions ago (it was changed in #447). The reason is mostly that Godot's resource files (.tres) can easily be injected with code, which can potentially be very dangerous in the wrong hands. Basically, someone can create a malicious .tres file and give it to someone else, pretending it's an innocent Pixelorama palette file. See this for more info godotengine/godot-proposals#4925

I was hoping they would have found a way to resolve this by now, but I don't think it's wise to wait for Godot (ba dum tss). And to be honest, we don't really need to use resource files. The only benefit is less code for loading/saving, but it's not that big of a difference. Plus, json files are easier to read since they have less useless text in them, and are easier to parse by third-party software.

The bad news, however, is that .tres palette files will not work in Pixelorama v1.0. And we can't keep compatibility, since giving users the ability to load .tres files kind of defeats the whole point. We don't want .tres files to be loaded at all. Since v1.0 is meant to be a breaking change, this is isn't the end of the world. To mitigate this, this PR also added an option to export palettes as image files. This option has also been added to 0.x and will be included in v0.11.4, so users with palettes made in 0.x can export them as images, which can be imported in v1.0.

All of the default palettes have been converted to tres to json, so the default experience of Pixelorama does not change.

This PR also attempts to simplify the code a bit, and adds some static typing improvements.

@Zireael07
Copy link

Hmm... it could be possible to parse specific properties from a tres file and just drop anything that doesn't fit (so if a value is expected to be float, and is code, or something else, just ignore it)

@dphaldes
Copy link
Contributor

dphaldes commented Dec 22, 2023

Why do you want to use a tres file specifically? Is there a reason? Using something like json is more portable imo. Tons of libraries to handle parsing/serialization/deserialization and thus adding support to external tools/libraries would be easier with json

@Zireael07
Copy link

@mystchonky Tres was used before this PR, that's why I asked about some minimal compatibility.

Good point about json being more portable/universal, though.

@dphaldes
Copy link
Contributor

We could have an external tool to convert the old palettes to the new system for a while.

@OverloadedOrama
Copy link
Member Author

Hmm... it could be possible to parse specific properties from a tres file and just drop anything that doesn't fit (so if a value is expected to be float, and is code, or something else, just ignore it)

Indeed, not sure how easy that would be though. Perhaps we could attempt it after this PR is merged. For now, v0.11.4 (the next version of 0.x) will have the option of exporting palettes as png files, which can be imported in v1.0, as a way to easily convert between the old palettes and the new.

@Zireael07
Copy link

Perhaps we could attempt it after this PR is merged

Definitely a follow-up PR, wasn't saying it should be done here. This PR is already a large undertaking

@OverloadedOrama OverloadedOrama merged commit 0cb98f6 into Orama-Interactive:master Dec 22, 2023
4 checks passed
@OverloadedOrama OverloadedOrama deleted the palette-json branch December 22, 2023 22:28
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.

3 participants