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

Allow style bindings when using the feature rendering cache #6964

Merged
merged 6 commits into from
Dec 15, 2023

Conversation

geographika
Copy link
Member

Pull request to address the issues raised in #6963.

This adds additional checks to ensure OUTLINEWIDTH and road/line casing is applied if attribute binding is used for OUTLINEWIDTH, and that the correct bound WIDTH is used for rendering.

The approach is to apply the binding before calculating the underlying line style, and then avoid rebinding this to preserve the style in msDrawVectorLayer.

From initial tests it no longer seems necessary to disable caching when attribute binding is used in subsequent styles.
I added a test for #3976 and removed the check in c63553e that disables caching when
bindings are used for multiple styles. This part can easily be rolled back.

The changes maybe addresses some of the points raised in #3995. I'm unsure how legend creation is meant to work when using attribute binding - if each shape can be rendered differently, what should the legend display?

Reviews and comments very much appreciated as the rendering process is complex!

@sdlime
Copy link
Member

sdlime commented Nov 3, 2023

Will definitely take some time to review!

@sdlime
Copy link
Member

sdlime commented Nov 16, 2023

Regarding legends, it's basically impossible to generate legend icons with bindings because of all the variability and lack of access to features. I think it just punts and returns a blank image. Easy enough to test, just run a mode=legend with your test mapfile.

@geographika
Copy link
Member Author

@sdlime - when using bindings and legends, any of the bound attributes of the style will use the style defaults defined in initStyle.

For example width will default to 1. Colours will be be -1 -1 -1 255 and then ignored when generating the legend due to the MS_VALID_COLOR(style->color)) check. Legends using bindings for styles will therefore be blank.

I discussed this pull request briefly with @rouault at the Vienna codesprint - he sees no issues from a coding-side, but is less familiar with the logic around the rendering cache.

--

As an aside while debugging I had to comment out the following line as it always returned NULL, even though an IMAGETYPE was set on the map, and map->outputformat is set.

MapServer/src/maplegend.c

Lines 721 to 723 in 990d28e

if (!MS_RENDERER_PLUGIN(map->outputformat)) {
msSetError(MS_MISCERR, "unsupported output format", "msDrawLegend()");
return NULL;

The MS_RENDERER_PLUGIN macro expands to ((map->outputformat)->renderer) and the ((map->outputformat)->renderer) value was 105 so I don't know why it was always False. The msautotests in /renderers/legend.map pass when using a compiled mapserv so I guess this is a strange Visual Studio debugging issue?

@sdlime
Copy link
Member

sdlime commented Nov 27, 2023

Yes, this is old and somewhat complicated logic. Caching was mostly related to GD and when that support was yanked. I wonder if some remnants still exist.

@geographika
Copy link
Member Author

Caching was mostly related to GD and when that support was yanked. I wonder if some remnants still exist.

@sdlime the caching here only seems to be relevant to line layers to allow multiple overlain styles, and was added in bcaae2c

The GD caching (also in that commit) and using the variable gdImagePtr img_cache=NULL; seems to have been completely removed.

I've been testing this branch for a month with no issues, and if anything arises I'm happy to fix it and add a new test. Any further comments or is it ok to merge this in a few days time?

@sdlime
Copy link
Member

sdlime commented Dec 14, 2023

+1 on merging...

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.

None yet

2 participants