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

Removed fallback on web for drawAtlas #1153

Merged
merged 8 commits into from
Dec 7, 2021
Merged

Conversation

wolfenrain
Copy link
Contributor

Description

Initial testing if the fallback for the drawAtlas method is still needed for the web.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors and are passing (See Contributor Guide).
  • My PR does not decrease the code coverage, or I have a very special case and explained on the PR description why this PR decreases the coverage.
  • I updated/added relevant documentation (doc comments with ///) and updated/added examples in doc/examples.
  • I have formatted my code with flutter format and the flutter analyze does not report any problems.
  • I read and followed the Flame Style Guide.
  • I have added a description of the change under [next] in CHANGELOG.md.
  • I removed the Draft status, by clicking on the Ready for review button in this PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flame users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md).
  • No, this is not a breaking change.

Related Issues

Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR.

@luanpotter
Copy link
Member

I tested this on chrome mobile mode on desktop.

main:
image

testing-draw-atlas-on-web:
image

I believe this means:

  • both the fallback and the normal impl works on web now
  • so the fallback (that that doesn't work on devices) is no longer needed
  • however the normal implementation clearly shows the tile gap issue for the particular size/zoom I used. but that is a known issue unrelated to anything that the fallback just happened to fix.

@spydon
Copy link
Member

spydon commented Dec 5, 2021

I tested this on chrome mobile mode on desktop.

main:
image

testing-draw-atlas-on-web:
image

I believe this means:

  • both the fallback and the normal impl works on web now
  • so the fallback (that that doesn't work on devices) is no longer needed
  • however the normal implementation clearly shows the tile gap issue for the particular size/zoom I used. but that is a known issue unrelated to anything that the fallback just happened to fix.

Maybe we can leave it as a selectable mode called something else then?

@spydon spydon marked this pull request as ready for review December 7, 2021 14:49
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

@spydon spydon merged commit bd68220 into main Dec 7, 2021
@spydon spydon deleted the testing-draw-atlas-on-web branch December 7, 2021 21:42
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.

None yet

4 participants