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!: Dart SDK compatibility #77

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

benni-tec
Copy link

@benni-tec benni-tec commented Jan 16, 2024

Description

This PR makes this package compatible with the Dart SDK. In order to achieve this I changed 3 things:

  • Replace dart:ui's Color with a simple ColorData class
  • Replaced Rect from flame with Rectangle from dart:math
  • Replaced flutter_test with test package

The ColorData class is used excatly once for which I prepared a flame_tiled PR (however I need to wait until this is merged right) including conversion extensions.

No Rectangle attribute is ever read in flame_tiled, evenso flame already contains conversion extensions!

Checklist

  • The title of my PR starts with a [Conventional Commit] prefix (fix:, feat:, docs: etc).
  • I have read the [Contributor Guide] and followed the process outlined for submitting PRs.
    --> melos run analyze fails due to XmlData.value however I think this requires another issue/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.
    --> I don't think there are any exmaples necessary for this

Breaking Change

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

I think this should result in a minor version bump to 0.11.0 to indicate these breaking changes!

In order to achieve dart comaptibilty only two things changed:

  • Color: Custom data class --> flame_tiled contains a conversion extension
  • Rect: Instead using dart:math's Rectangle --> Use flame's toRect() method to convert

Related Issues

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!

packages/tiled/pubspec.yaml Outdated Show resolved Hide resolved
/// Basic data class holding a Color in ARGB format.
/// This can be converted to dart:ui's Color using the flame_tiled package
class ColorData {
static int _sub(int hex, int index) => (hex >> index * 8) & 0x000000ff;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just saw this in the other PR; is there a mix up between the two? If so; my comment there is valid here as well: why not store the "hex" as one value and do bit shifting in setters?

This is what Color was doing before and so the pattern would match and the memory footprint should be the same.

Copy link
Author

Choose a reason for hiding this comment

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

Yes you are right that would be more efficient! This change does belong here, but I based the PR‘s branch on this one so I don‘t have to integrate the new types later!

green = _sub(hex, 1),
blue = _sub(hex, 0);

const ColorData.rgb(this.red, this.green, this.blue, [this.alpha = 255])
Copy link
Collaborator

Choose a reason for hiding this comment

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

"rgba"? and why deviate from "argb"?

Copy link
Author

Choose a reason for hiding this comment

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

I don’t really know, it just felt more natural. For most colors the alpha value is just 0xff so it can have a default value at the end. CSS also puts it at the end #rrggbbaa, I believe for the same reason.

This constructor is meant for users and not used internally, so I think this way is a better experience. However I‘m happy to change it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I was trying to stick with the dart:ui color for familiarity.

@spydon spydon changed the title feat!: Dart SDK comaptibility feat!: Dart SDK compatibility Jan 18, 2024
@kevmoo
Copy link

kevmoo commented Jun 18, 2024

Would this help here? https://flutter.dev/go/move-flutter-agnostic-types

If yes, help me understand how.
If not, what's missing?

@spydon
Copy link
Member

spydon commented Jun 19, 2024

Would this help here? https://flutter.dev/go/move-flutter-agnostic-types

If yes, help me understand how.
If not, what's missing?

Yes! That would be exactly what we need here, it migrates all the types that we need out of flutter.

Thanks!

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