Skip to content

Conversation

tapycha
Copy link

@tapycha tapycha commented Sep 22, 2025

Related issue

VectorLayerModule declares 4 callbacks (OnVectorMeshCreated, OnVectorMeshDestroyed, OnVectorMeshTurnVisible, OnVectorMeshTurnInvisible) but implements only 1 (OnVectorMeshCreated)

Description of changes

  • Modified callbacks return values to include CanonicalTileId as a parameter, as a common tile identifier
  • In the CreateVisual method in _meshGenerationUnit.MeshGeneration callback replaced if-else with switch for ease of reading
  • Added _visibleTiles hashset to track visible tiles for OnVectorMeshTurnVisible and OnVectorMeshTurnInvisible callbacks(could be done without it with a bit bigger rewrite of UpdateActiveTileList)
  • UpdateVisibilityCallbacks, which triggers OnVectorMeshTurnVisible and OnVectorMeshTurnInvisible callbacks when the tile status change
  • OnVectorMeshDestroyed callback call added to ClearDisposedDataVisual (seems like a logical place for it, but needs validation)

QA checklists

  • Add relevant code comments. Every API class and method should have <summary> description as well as description of parameters.
  • Add tests for new/changed/updated classes and methods!!!
  • Check out conventions in CONTRIBUTING.md.
  • Check out conventions in CODING-STYLE.md
  • Update the changelog
  • Update documentation.

Reviewers

Tag your reviewer(s). Choose wisely.

@brnkhy
Copy link
Contributor

brnkhy commented Sep 22, 2025

this is a little more tricky.
those events are probably leftovers from previous iteration of the class, created and destroyed should be valuable but on top of my head; I'm not so sure about the visibility ones so it might be more preferable to remove those events instead of adding more complexity and checks (performance) to the procedure.
I'll look into it properly some time though, I don't really like 2-3 layers in vector module anyway so there's a lot more work to do there in the future.

@tapycha
Copy link
Author

tapycha commented Sep 23, 2025

Well, there is logic behind it as Create/Destory are kinda adding/removing tiles from the pool and Visiblie/Invisible show/hide them as needed. I made this pr so there is at least some implementation for those callbacks.
Regarding performance, there are already 2 checks in hashsets and 1 more doesn't hit performance much.
Reworking layers looks like a big task that will require a lot of work. The client that I work for wants to use MapBox 3.0 in production and I would like to help with stuff that is quick to do and can deliver the most impact to performance and ease of use with MapBox.
I identified some performance issues with:

  • UnityTileImageContainer.SetImageData constantly does Delegate Combine/Remove which creates a lot of GC.Alloc and eats cpu cycles(quite bad one)
  • ResilientWebRequestFileSource.CreateFinalUrl does a lot of string.Concat which creates 2.6KB GC.Alloc (I think this can be reduced)
  • MapboxContext.GetSkuToken generates 112 bytes of GC.Alloc each time is called and I think can be cached(not much but still it's memory)

I will make a new pr regarding UnityTileImageContainer.SetImageData with tests and a potential solution.

@brnkhy
Copy link
Contributor

brnkhy commented Sep 23, 2025

@tapycha ah I wasn't saying it's a b'g performance hit or anything. Layer/Tag modifiers were things that doesn't have any impact on system, if you don't use them nothing would change for you. This one though touches a core loop and will run thousands of times in a few seconds, so I'll have to test and check it out properly.

  • ResilientWebRequestFileSource.CreateFinalUrl does a lot of string.Concat which creates 2.6KB GC.Alloc
    I think it's also similar where we create file paths as well, if I remember correct I used safest way possible for both (uri builder classes etc) even though they are little on the slower side just to be safe and not having to deal with localization/culture etc issues. I haven't checked the CreateFinalUrl yet and I will, there's always a chance I left over a lazy concat of course.

  • MapboxContext.GetSkuToken generates 112 bytes of GC.Alloc each time is called and I think can be cached
    that's a business related thing I would prefer to leave alone. I think SkuToken has time element in it and refreshes by an interval or something so can't cache it outside the core algorithm.

  • UnityTileImageContainer.SetImageData constantly does Delegate Combine/Remove which creates a lot of GC.Alloc and eats cpu cycles(quite bad one)
    I think that dispose event is a recent change to notify system that a texture is getting destroyed and any tiles in cache using that texture should be invalidated. this prevents system using tiles from cache with missing textures. there should be a better way to implement this though, maybe simply checking if tile is good-to-reuse without any events or anything is the cleanest and the best option.

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.

2 participants