-
Notifications
You must be signed in to change notification settings - Fork 197
MAPSJS-2983 Write to the console if we need too many attempts getting… #2284
Conversation
70907f7
to
117c40c
Compare
Codecov Report
@@ Coverage Diff @@
## master #2284 +/- ##
==========================================
+ Coverage 67.95% 68.25% +0.30%
==========================================
Files 315 315
Lines 27766 27763 -3
Branches 6215 6214 -1
==========================================
+ Hits 18869 18951 +82
+ Misses 8897 8812 -85
Continue to review full report at Codecov.
|
// There is a texture missing in the cache, we attempt again, and then error out. | ||
const missingTextureCount = missingTextureName.get(imageTextureName); | ||
missingTextureName.set(imageTextureName, missingTextureCount ? missingTextureCount + 1 : 0); | ||
if (missingTextureName.get(imageTextureName)! >= SEARCH_CACHE_ATTEMPTS) { |
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.
We could log the error just once to avoid cluttering the console. Not sure if it makes sense to do it after n attempts or at the first one. It's difficult to say when "n attempts" is enough, cause it depends on how many POIs use that same image.
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 would argue that if there is an error after 5 attempts, there is something fundamentally wrong and a new approach should be taken (like the ones I mentioned in the commit message). At some point, trying to present the error in a nice way is counter productive.
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.
In saying that, I can reduce cluttering the console by changing the >=
to be just =
, then we avoid any complex state management.
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 left some comments.
7b43bd8
to
7e51b07
Compare
@@ -396,11 +396,10 @@ function findImageItem( | |||
// There is a texture missing in the cache, we attempt again, and then error out. | |||
const missingTextureCount = missingTextureName.get(imageTextureName); | |||
missingTextureName.set(imageTextureName, missingTextureCount ? missingTextureCount + 1 : 0); | |||
if (missingTextureName.get(imageTextureName)! >= SEARCH_CACHE_ATTEMPTS) { | |||
if (missingTextureName.get(imageTextureName)! === SEARCH_CACHE_ATTEMPTS) { |
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.
minor: we could do missingTextureName.get(imageTextureName)
only once.
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.
Yes, but I would need to then do the +1 twice. What is preferred?
const webGLRenderer = { | ||
capabilities: { | ||
isWebGL2: false | ||
} | ||
} as THREE.WebGLRenderer; | ||
const mapViewStub = sinon.createStubInstance(MapView); | ||
const poiManager = new PoiManager((mapViewStub as any) as MapView); | ||
const testImageName = "testImage"; | ||
const mapEnv = new Env(); |
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.
move this to a beforeEach() function. Each test should start with a clean slate.
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.
Yes, the MapEnv is actually never used, but I will do it to make the test at least easy to extend in future.
) => { | ||
// Note, the images must be unique, otherwise the test will fail, because the internal | ||
// caching mechanism of the ImageCache will have already loaded the image. | ||
return testCache.addImage(name, `https://picsum.photos/${x++}/${y++}`, load); |
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.
Unit tests must not have external dependencies. We can instead stub testCache's findImageByName method and make it return what we want whenever we want in the test.
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.
Done, I could use a local resource so I don't need to stub.
de21974
to
e0aaff6
Compare
… the image from the cache Some failed attempts are ok, because there are cases where the DataSource is attached and ready and the map has started to render, but the theme isn't fully set. There are other ways to fix the problem, but they are generally more invasive and could introduce regressions / performance issues, some options are: - In the TileGeometryCreator::createAllGeometries method, await on the getTheme() Promise - Don't allow new TextElements to be added while the theme is updating - Investigate and remove all calls to update the mapview while the method `addDataSource` is called and see if this can be removed, because no update calls means we don't render and request new tiles which may contain references to POI's that don't yet exist on the cache. For the sake of simplicity, this is the preferred method. Change-Id: I441a0ff5f63fd3d2a403b43b422c3044c902c7a6 Signed-off-by: Jonathan Stichbury <[email protected]>
e0aaff6
to
68c1f11
Compare
Signed-off-by: Jonathan Stichbury <[email protected]>
Signed-off-by: Jonathan Stichbury <[email protected]>
Signed-off-by: Jonathan Stichbury <[email protected]>
…source Signed-off-by: Jonathan Stichbury <[email protected]>
68c1f11
to
715bad5
Compare
#2284) Some failed attempts are ok, because there are cases where the DataSource is attached and ready and the map has started to render, but the theme isn't fully set. There are other ways to fix the problem, but they are generally more invasive and could introduce regressions / performance issues, some options are: - In the TileGeometryCreator::createAllGeometries method, await on the getTheme() Promise - Don't allow new TextElements to be added while the theme is updating - Investigate and remove all calls to update the mapview while the method `addDataSource` is called and see if this can be removed, because no update calls means we don't render and request new tiles which may contain references to POI's that don't yet exist on the cache. For the sake of simplicity, this is the preferred method. Signed-off-by: Jonathan Stichbury <[email protected]> Signed-off-by: Fischer, Thomas <[email protected]>
… the image from the cache
Some failed attempts is ok, because there are cases where the DataSource is attached and ready
and the map has started to render, but the theme isn't fully set.
There are other ways to fix the problem, but they are generally more invasive and could introduce
regressions, some options are:
addDataSource
is calledand see if this can be removed, because no update calls means we don't render and request new tiles
which may contain references to POI's that don't yet exist on the cache.
But, for the sake of simplicity, this is the preferred method.