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: Support for new atlas format and rotated sprites #3097

Conversation

Yayo-Arellano
Copy link
Contributor

Description

Added support to load new atlas format and support for rotated sprites

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

…iginalWidth, originalHeight, degrees, rotate, index. So we set the defaults in code
@Yayo-Arellano Yayo-Arellano force-pushed the add_support_to_rotation_and_new_file_format branch 4 times, most recently from bb3f6c6 to 5f48e6d Compare March 25, 2024 14:33
@spydon spydon changed the title feat: Added support to load new atlas format and support for rotated sprites feat: Support for new atlas format and rotated sprites Mar 25, 2024
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Looks good, just a few small comments

Comment on lines 12 to 13
flame_texturepacker:
path: ../
Copy link
Member

Choose a reason for hiding this comment

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

This should not be modified, Melos keeps track of this through the pubspec_overrides.yaml file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I removed the commit

@@ -297,3 +296,14 @@ extension _IteratorExtension on Iterator<String> {
return null;
}
}

extension _IterableExtension<T> on Iterable<T> {
T? firstWhereOrNull(bool Function(T element) test) {
Copy link
Member

Choose a reason for hiding this comment

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

This is already built-in to collection:
https://api.flutter.dev/flutter/package-collection_collection/IterableExtension/firstWhereOrNull.html

So just import collection instead, since we use that everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I updated the code to use the build-in collection

) {
_decorator = Transform2DDecorator(_transform);
if (region.rotate) {
_transform.angle = radians(90);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_transform.angle = radians(90);
_transform.angle = math.pi / 2;
Suggested change
_transform.angle = radians(90);
_transform.angle = tau / 4;

We should limit the use of degrees as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Code updated

static final _tmpRenderSize = Vector2.zero();

@override
void render(
Copy link
Member

Choose a reason for hiding this comment

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

Sound all this be skipped if there is no rotation set?
So that super.render is called if there is no rotation, since that should be more performant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good idea. I updated the code

@Yayo-Arellano Yayo-Arellano force-pushed the add_support_to_rotation_and_new_file_format branch from 5f48e6d to b731530 Compare March 26, 2024 01:17
@Yayo-Arellano Yayo-Arellano force-pushed the add_support_to_rotation_and_new_file_format branch from b731530 to 5449f1a Compare March 26, 2024 01:19
@Yayo-Arellano Yayo-Arellano force-pushed the add_support_to_rotation_and_new_file_format branch from 5449f1a to 55286c2 Compare March 26, 2024 01:27
@Yayo-Arellano
Copy link
Contributor Author

@spydon Hello. I have updated/replied to the comments. Please take a look when you have some time.

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm!

@spydon spydon enabled auto-merge (squash) March 26, 2024 09:04
@Yayo-Arellano
Copy link
Contributor Author

@spydon There is a broken test case. But the code is not related to this pull request. Can you take a look?

@spydon spydon merged commit ed690b3 into flame-engine:main Mar 26, 2024
8 checks passed
@spydon
Copy link
Member

spydon commented Mar 26, 2024

@spydon There is a broken test case. But the code is not related to this pull request. Can you take a look?

It was the annoying flaky test that was acting up again, re-ran it and now the PR is merged. :)

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