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

Patch - Mehah viewportcontrol #1097

Closed
wants to merge 8 commits into from
Closed

Patch - Mehah viewportcontrol #1097

wants to merge 8 commits into from

Conversation

4drik
Copy link
Contributor

@4drik 4drik commented Jul 5, 2020

A review of viewportcontrol by mehah

@diath
Copy link
Collaborator

diath commented Jul 5, 2020

Thanks for taking the time to make this change a proper PR, could you perhaps rename the files viewmapcontrol to mapviewcontrol and the class from ViewportControl to MapViewControl? This way we could keep it consistent with other map related classes.

@iryont
Copy link
Collaborator

iryont commented Jul 5, 2020

Personally I believe there are a lot of issues with this approach / code such as:

  1. Aware range is static now defined by constant values.
  2. ViewportControl class uses aware range while it should use drawing dimension of the MapView it belongs to.
  3. ViewportControl class is not required, the basic idea (drawing less tiles depending on the walk direction) can be included within MapView class.
  4. All of it should be done within updateVisibleTilesCache and not within drawing logic function which adds unnecessary indirection and logic to be calculated each frame.

To be honest all of it seems to be over-complicated. Please note this is my personal opinion.

@4drik
Copy link
Contributor Author

4drik commented Jul 5, 2020

@diath Bakasura changes of PR:
#1098

@4drik 4drik closed this Jul 5, 2020
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.

3 participants