-
-
Notifications
You must be signed in to change notification settings - Fork 976
Refactor SourceCache to TileManager #6635
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
Conversation
|
💥 |
|
|
||
| SourceCache.maxOverzooming = 10; | ||
| SourceCache.maxUnderzooming = 3; | ||
| TileManager.maxOverzooming = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these defined here? are these static members?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A long running question I have had. The variables are also named backwards fyi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move them inside the class and add a static modifier to them? I think this is a old javascript way of doing static variables...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that you merged the PR so consider doing this in a different PR then...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't see these above! I'm about to open another PR in the next 5 min though... it should be draft a while so we can do it there... same file
|
I've added mostly minor comments, thanks for making this happen! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6635 +/- ##
=======================================
Coverage 96.29% 96.29%
=======================================
Files 293 293
Lines 35475 35481 +6
Branches 8688 8691 +3
=======================================
+ Hits 34160 34166 +6
Misses 1315 1315 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I had a radical idea to make everything that is not used outside the class "private". |
|
Nice work! Thanks! |
|
BOOM? |
|
BOOM! |
* webstorm refactor - change filename to tile_manager.ts - passing * webstorm refactor - move files to tile directory - passing * webstorm replace - word boundary lowercase readme and comments - passing * webstorm replace - word boundary camelCase - passing * webstorm replace - word boundary camelCase plural - passing * rename pauseSource to pauseTiles * webstorm replace - word boundary ProperCase - passing * comment - ProperCase plural * webstorm replace - no word boundary ProperCase - passing * webstorm replace - no word boundary camelCase - passing * local vars rename, comments * comments and strings with space * changelog * draw_fill and map local vars, revert old changelog entries * test string * address review comments * fix terrain test * Apply suggestion from @HarelM --------- Co-authored-by: wayofthefuture <wayofthefuture@users.noreply.github.com> Co-authored-by: Harel M <harel.mazor@gmail.com>
Summary
This PR refactors the
SourceCacheclass toTileManagerto better reflect its actual responsibilities within the rendering pipeline.Rationale
The name
SourceCachehas been misleading and it significantly understates what this class actually does. After considering alternatives likeSourceControllerandSourceManager, I believeTileManagerto be the most accurate name.What This Class Actually Does
Beyond simple caching:
Painterduring render passes, provides renderable tile IDs to drawing functionsWhy "TileManager"?
TileobjectsImplementation & Notes
SourceCachetoTileManagersourceCache→tileManager)tile_manager.tstilethat includestile_manageras well as the adjacenttile_files.sourcedirectory andtilesdirectory.