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

feat(fire_atlas): Encoded option to load json instead of .fa #2649

Merged
merged 8 commits into from
Sep 18, 2023

Conversation

Arenukvern
Copy link
Contributor

@Arenukvern Arenukvern commented Aug 13, 2023

Description

Hello.
I would like to suggest to add option to load atlas without compression as simple json file.
I found it useful to use for example in itch.io, where gzip encoder causes an error during decoding.
Please notice is just a draft, if it is a case that this change can be useful - I will add all relevant documentation and tests if needed.

Thank you for your time!

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

@Arenukvern Arenukvern changed the title DRAFT: feat(fire_atlas): encoded option to load json instead of .fa feat(fire_atlas): encoded option to load json instead of .fa Aug 17, 2023
@spydon spydon changed the title feat(fire_atlas): encoded option to load json instead of .fa feat(fire_atlas): Encoded option to load json instead of .fa Aug 17, 2023
Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great idea! The changes in place right now also look good, I think we should just address this github warning about path not being a dependency of the project, and we should maybe also add a method that saves the atlas as json?

@Arenukvern
Copy link
Contributor Author

@erickzanardo understood, thank you for your review!
Will make the changes (path & save the atlas as json) this week

@Arenukvern
Copy link
Contributor Author

@erickzanardo
made changes, could you please take a look?

Copy link
Member

@luanpotter luanpotter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one consideration

@spydon
Copy link
Member

spydon commented Sep 15, 2023

@Arenukvern do you want to finish this up with that last comment? If not we'll take it over and fix it. :)

@spydon spydon enabled auto-merge (squash) September 18, 2023 11:39
@spydon spydon merged commit 5be6fc8 into flame-engine:main Sep 18, 2023
7 checks passed
@Arenukvern
Copy link
Contributor Author

@Arenukvern do you want to finish this up with that last comment? If not we'll take it over and fix it. :)

sorry for late answer, got sick and completely lost free time

@spydon
Copy link
Member

spydon commented Sep 20, 2023

sorry for late answer, got sick and completely lost free time

No worries, hope you're feeling better now!
The PR is merged and will be included in the next release. :)

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.

4 participants