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

Add clone() methods to AsepriteSpriteSheet and AsepriteResource #127

Merged
merged 5 commits into from
Aug 27, 2022

Conversation

mattjennings
Copy link
Contributor

@mattjennings mattjennings commented Aug 26, 2022

===:clipboard: PR Checklist :clipboard:===

  • 📌 issue exists in github for these changes
  • 🔬 existing tests still pass
  • 🙈 code conforms to the style guide
  • 📐 new tests written and passing / old tests updated with new scenario(s)
  • 📄 changelog entry added (or not needed)

==================

Closes #126

Changes:

  • Added clone() method to AsepriteSpriteSheet
  • Added clone() method to AsepriteResource

The integration tests are failing for me on main:

Test: A Aseprite parsed resource
        Expect: Aseprite Loop Animation
[ERROR]: Image ./test/integration/images/expected.png did not match acutal ./test/integration/images/actual.png, 146 different!

but unit tests all pass.

I'm also willing to revert the commits for adding clone to AsepriteResource, but if we're okay with how it's implemented, I quite like having it. Cloning the resource over digging into the spritesheet/anims is much cleaner in usage.

@eonarheim
Copy link
Member

@mattjennings I'll look into the integration test, I wouldn't have expected your changes to cause the failure. Possibly a browser/platform rendering difference? Looks like it passes in CI though.

@mattjennings
Copy link
Contributor Author

If it passes in CI then great!

Yeah, I am not too sure what causes it. Smells like curse of the M1 Mac again... is there a way to know which browser ex-test uses?

Here are the expected/actual/diff images it produced:

expected
actual
diff-actual

@eonarheim
Copy link
Member

@mattjennings Agree, it's probably that M1 Mac issue we ran into on the main repo.

You ready for me to merge with CI passing?

is there a way to know which browser ex-test uses?

This is a good question, I'll poke around 👍

@mattjennings
Copy link
Contributor Author

Yup it's good to go!

@eonarheim eonarheim merged commit 1b27b9e into excaliburjs:main Aug 27, 2022
@eonarheim
Copy link
Member

@mattjennings Thanks again!

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.

Add a way to get a new spritesheet/animation reference from resource
2 participants