Replace BitmapPath with BitmapId #2632
Conversation
|
I had a rough look into this pull request and I like the changes being made. Especially that now the Images are cached by uri so that the uri is the id of the images/sprites/icons ..... I like the ImageFetcher too so there is a seperetion between loading and displaying like in the render Pipeline. |
| @@ -27,7 +27,7 @@ public void Dispose() | |||
| SymbolCache.Dispose(); | |||
| VectorCache.Dispose(); | |||
| TileCache.Dispose(); | |||
| BitmapRegistry.Dispose(); | |||
| ImagePathCache.Dispose(); | |||
There was a problem hiding this comment.
Decided to rename from BitmapPath to ImagePath because svg is not a bitmap and already started to use this in some places.
btw, I am also considering a data:// schema to allow an svg string. Or maybe it should be svgstring:// or svg://. With this change ImagePath will also not be fully correct. ImageSource might be better.
There was a problem hiding this comment.
The change is good, but ImagePath isn't good for this.
Regarding the correct naming: Up to now this property is used for the source (file, http and so on). You don't say anything about the content. So it should be ImageSource or Source.
Regarding SVG: When you add a svg: source, you combine the source with the type. That isn't logical. And a SVG could be an embedded resource, a file or coming via http. Also a string is possible. So a string and base64 source would be better. And there could be even more sources.
Mapsui core should be responsible for loading, the renderer for interpreting the content.
There was a problem hiding this comment.
@charlenni Good point. All other schemes are source-types and svg is an image-type, so in that sense 'string' would be better, because we are using a string as source. But this would only work for svg and I would want to make that clear. So perhaps it should be svgstring.
And though it is always possible to put the svg in file or embedded resource it would be easier in some cases if you just could just copy and paste the string. Then again this functionality may not be that important. I'll sleep on it.
|
|
||
| [return: NotNullIfNotNull(nameof(key))] | ||
| public SKImage GetOrCreatePaint(string key, Func<SKImage> toSKImage) | ||
| public T GetOrCreateSKObject<T>(string key, Func<T> toSKObject) where T : SKObject |
There was a problem hiding this comment.
Reusing SpriteCache to get CalloutRenderer to work. This is temporary and should be fixed before the next beta.
| @@ -24,10 +23,10 @@ namespace Mapsui.Rendering.Skia; | |||
|
|
|||
| public sealed class MapRenderer : IRenderer, IDisposable | |||
| { | |||
| private readonly DisposableWrapper<IRenderService> _renderService; | |||
| private readonly IRenderService _renderService; | |||
There was a problem hiding this comment.
We should put effort in trying to keep disposability simple. When possible there should be a single class responsible for creating and disposing object. In most cases this will be a cache.
I also would like to keep the render caches encapsulated within the render. They are performance optimizations of the a specific renderer and it would be better if there is no need to know anything about them on the outside.
| { | ||
| _renderService = new DisposableWrapper<IRenderService>(new RenderService(), true); | ||
| // Todo: Think about an alternative to initialize. Perhaps the capacity should |
There was a problem hiding this comment.
User should not have to know how many features will be drawn by the renderer.
An alternative approach is used in the TileLayer. There all the tiles needed in a single iteration are always in cache. In the TileLayer we keep track of how many tiles are needed. MemoryCache.MinTiles is updated on every iteration.
A better solution may be to add the iterationCount parameter to cache methods so that the cache can know what is needed in a single render cycle.
| if (calloutStyle.Invalidated) | ||
| { | ||
| // Todo: Move this update to the callout itself | ||
| calloutStyle.ContentId = Guid.NewGuid().ToString(); |
There was a problem hiding this comment.
The calloutStyleRenderer heavily used the BitmapRegistry. It was tough to rewrite. The guid ids may have to go. Perhaps we should use an Id like in BaseFeature.
private static long NextId()
{
return Interlocked.Increment(ref _currentFeatureId);
}
Another option is to keep the style immutable and you would have to assign a new style to change it.
There was a problem hiding this comment.
It is, because, if you change anything in the callout content, you have to create a new callout to cache. It isn't logical to create a new style to change the header string or the font size of the detail entry.
There was a problem hiding this comment.
Btw. your comment is wrong. The callout is cached, so that it has not to be created each time. It has nothing to do with the CalloutStyle. It is a CalloutStyleRenderer only thing.
| @@ -18,7 +19,7 @@ public enum CalloutType | |||
| /// <summary> | |||
| /// Content is custom, the bitmap given in Content is shown | |||
| /// </summary> | |||
| Custom, | |||
| Image, | |||
There was a problem hiding this comment.
Rename CalloutType.Custom to Image because it is currently not possible anymore to use an in memory SKPicture as bitmap, just a BitmapPath image. Perhaps there should be no types at all just options to use an Image or Detail or not.
There was a problem hiding this comment.
There are 3 types of callouts: a single line, a header detail combination and an user drawn.
There was a problem hiding this comment.
If I think about the callout type, there could be a fourth type, an owner 'drawn' style. This provides two callbacks. One for getting the size of the content and one to draw the content. With this it should be possible to show normal UI elements in the callout. If you have to draw the callout, you get a screen position and a clip rect, where you have to display the normal UI elements. You need the clip rect when the callout is at the border of the map control.
pauldendulk
changed the title
That is good to hear. It was one of the biggest changes I made in one PR and unfortunately I had to break a lot. I am convinced it will make things a lot better in the long run (it also allows some additional changes in upcoming PRs) but there will be frustrations because of breaking changes and I will have introduced some bugs. About the ImageFetcher. Yes, this is the kind of thing I do a lot these days. I separate simple logic from stateful classes. The simple logic can often be pure functions. The ImageFetcher is not strictly a pure function because it depends on remote infrastructure but it is a method that is easy to understand. You put something in and something comes out. The ImagePathCache remains stateful, that is still a problem, but it is easier to see what it is about. The current CalloutStyleRenderer also has some methods that could be moved out of that class to public pure functions so that they could be reused. The code to render the bubble and the tail (that is how the pointy part is called according to chatgpt) is quite advanced, not easy to write yourself and could be useful in other places. A custom renderer which creates an SKPicture could easily wrap that into the bubble to turn it into a callout using the RenderCallout method. This separation of state and logic is what many developers think should be avoided because this seems to conflict with object oriented programming. |
|
Summary, this PR replaced the SymbolStyle.BitmapId with a BitmapPath. It had a lot of impact on a lot of things and there are more changes to come. The BitmapRegistry (now the ImagePathCache) only holds streams that represent bitmap or svg images, not SKImage or SKPicture, these things should be in a cache specific to the SkiaRenderer. |
|
As said in a code comment: ImagePathCache is totally useless in my eyes. You cache data, that no one needs. The renderer needs the image cache to draw the images fast. It converts the content in a format, that is capable of doing this. If the image changes, the ImagePath changes too. If the ImagePath doesn't change, the underlying converted renderable image doesn't change and the renderer hasn't to access the ImagePath a second time. So why to cache it? When is it useful to have a copy in memory? The only use case I could think about is, when you have two different renderers with two different caches for ready rendered ImagePathes. |
|
There is still a lot more work to do on the caches but some general remarks about what I have in mind. I see three levels in our process:
The ImagePathCache falls in the first layer and the and render cache in the second. This was a reason for me to treat is as a separate thing. Something else I had in mind is that we could always keep the ImagePathCache in memory because I expect this to always be a limited set (unless users start generating file sources on the fly in code), while the render cache is cleaned regularly depending on what was recently in view or not. Then again, that depends on the implementation, we could also make a one on one cache for the skia version of the images permanently in the cache. So, that argument may not fly. The main reason for doing it like this now is the logical organization that I have in mind. I am trying to organize it in a way that is easy to understand for me. Not sure if there is a real need for it in the end, but then it might still make more sense to organize it like that. Let's see how it turns out. |
|
I'm not sure, if the ImagePathCache (btw. a misleading name) is needed. You say, that there are not so much things to fetch than to render. But that isn't correct. Each thing to render has to be fetched, but could be used more times. And there isn't a thing, that could be rendered without fetching it before. If a thing have the same source, then it should be already in the render cache. No need for accessing the fetch cache. And what should be the reason to remove a rendered thing from the render cache, but not from the fetch cache? How do you determine, that the thing should be removed from the render cache. It could come again in the next rendering because the map is moving the next symbol in. So the question is, what could be done with fetched cache thing except transferring them to render cache? Tiles, symbols or brushes, all of them are used for rendering. |
No description provided.