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

Add support for using all OGR style parameters #4562

Closed
szekerest opened this Issue Jan 15, 2013 · 7 comments

Comments

Projects
None yet
3 participants
@szekerest
Member

szekerest commented Jan 15, 2013

Currently the OGR driver supports to get only of the various OGR label style parameters as special attributes (ie. OGR:LabelText or OGR_LabelAngle)

We would like to use further style parameters in mapserver so I've added a sample implementation here:
szekerest@b29c6f4

This implementation provides support to use the following special attributes

OGR:LabelParam[paramId]
OGR:BrushParam[paramId]
OGR:PenParam[paramId]
OGR:SymbolParam[paramId]

where [paramId] is an integer value according to the OGRSTLabelParam, OGRSTBrushParam, OGRSTPenParam, OGRSTSymbolParam enumerations in ogr_core.h.

With this addition we can easily map feature styles (ie mapinfo styles) to classes for example by using CLASSITEM [OGR:BrushParam2] which refers to the mapinfo brush id.

BTW: the ogr autostyle mechanism provides support to map style id-s to mapserver symbols but as of mapserver 6.0 this can be used less efficiently, since many of the symbol parameters (like patterns) has been moved to the style sections. So we'd prefer to map styleid-s to classes instead of symbols then.

@ghost ghost assigned szekerest Jan 15, 2013

@dmorissette

This comment has been minimized.

Show comment
Hide comment
@dmorissette

dmorissette Aug 5, 2013

Contributor

@szekerest I like the idea in general, the only thing that bothers me is the use of integer ids from ogr_core.h, since those ids are internal/arbitrary and were never guaranteed to never change over time (well, from now on OGR would be forced to keep them constant). Would the use of the actual param name affect performance or complicate the implementation significantly?

e.g. Instead of [OGR:BrushParam2] we could use [OGR:BrushParam:id]

Contributor

dmorissette commented Aug 5, 2013

@szekerest I like the idea in general, the only thing that bothers me is the use of integer ids from ogr_core.h, since those ids are internal/arbitrary and were never guaranteed to never change over time (well, from now on OGR would be forced to keep them constant). Would the use of the actual param name affect performance or complicate the implementation significantly?

e.g. Instead of [OGR:BrushParam2] we could use [OGR:BrushParam:id]

@szekerest

This comment has been minimized.

Show comment
Hide comment
@szekerest

szekerest Aug 5, 2013

Member

@dmorissette I don't see a function in OGR to map param names to the internal id-s and it would not be reasonable to maintain such mapping tables in mapserver, which would be specific to OGR versions as well. I think the change in the id-s are less likely happens due to the developers and it would not be a big issue to migrate the mapfiles when the OGR version is changing.

BTW: We should also consider changing the colons in the field names to some other character to make it sufficient to be used in the WxS XML responses. This also a problem with some other drivers.

Member

szekerest commented Aug 5, 2013

@dmorissette I don't see a function in OGR to map param names to the internal id-s and it would not be reasonable to maintain such mapping tables in mapserver, which would be specific to OGR versions as well. I think the change in the id-s are less likely happens due to the developers and it would not be a big issue to migrate the mapfiles when the OGR version is changing.

BTW: We should also consider changing the colons in the field names to some other character to make it sufficient to be used in the WxS XML responses. This also a problem with some other drivers.

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Aug 9, 2013

Member

Please give a status update as to wether this should really go into 6.4, as we really need to be releasing a first beta very soon. Maybe we can integrate this as-is for 6.4 and come back on it for 7.0 for the id issue, along with replacing colons with an xml compatible character (as this would be breaking backwards compatibility, 7.0 would be the time)

Member

tbonfort commented Aug 9, 2013

Please give a status update as to wether this should really go into 6.4, as we really need to be releasing a first beta very soon. Maybe we can integrate this as-is for 6.4 and come back on it for 7.0 for the id issue, along with replacing colons with an xml compatible character (as this would be breaking backwards compatibility, 7.0 would be the time)

@szekerest

This comment has been minimized.

Show comment
Hide comment
@szekerest

szekerest Aug 9, 2013

Member

OK, I'll review the code again today to check whether it could go ahead or not.

Member

szekerest commented Aug 9, 2013

OK, I'll review the code again today to check whether it could go ahead or not.

@dmorissette

This comment has been minimized.

Show comment
Hide comment
@dmorissette

dmorissette Aug 9, 2013

Contributor

Thanks for the explanation @szekerest, I understand the issue with respect to accessing the style param names. The names are available in static arrays of OGRStyleParamId in gdal/ogr/ogrfeaturestyle.cpp, so we'd need to make a change in OGR to either expose those arrays, or to provide some convenience functions through the OGR C API to access that info.

Sounds like numeric ids is the only way to go for now... I agree with @tbonfort about waiting for 7.0 to deal with the colons in field names, perhaps in a separate ticket.

Contributor

dmorissette commented Aug 9, 2013

Thanks for the explanation @szekerest, I understand the issue with respect to accessing the style param names. The names are available in static arrays of OGRStyleParamId in gdal/ogr/ogrfeaturestyle.cpp, so we'd need to make a change in OGR to either expose those arrays, or to provide some convenience functions through the OGR C API to access that info.

Sounds like numeric ids is the only way to go for now... I agree with @tbonfort about waiting for 7.0 to deal with the colons in field names, perhaps in a separate ticket.

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Sep 18, 2013

Member

this didn't make it into 6.4, demilestoning

Member

tbonfort commented Sep 18, 2013

this didn't make it into 6.4, demilestoning

@szekerest

This comment has been minimized.

Show comment
Hide comment
@szekerest

szekerest Mar 24, 2014

Member

This is already implemented. Field names with colons should be handled separately

Member

szekerest commented Mar 24, 2014

This is already implemented. Field names with colons should be handled separately

@szekerest szekerest closed this Mar 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment