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

Restore the WKT parser, example & tests #2172

Merged
merged 2 commits into from Jun 27, 2014

Conversation

eriktim
Copy link
Contributor

@eriktim eriktim commented Jun 6, 2014

This is a first attempt to restore the WKT parser.
It is probably preferred to rewrite the parser to the ol.format namespace as well,
but I would first like to have some confirmation on this before doing so.

@tschaub
Copy link
Member

tschaub commented Jun 6, 2014

Thanks for taking this on @GingerIK! We briefly discussed starting this from scratch and writing a tokenizer based parser. But I think your approach is probably the shortest path to success. Assuming you intend to continue adapting this so it extends from ol.format.TextFeature, I think this sounds good.

I'll let others weigh in, but I'd say go for it (as long as it conforms to the same interface, we can rewrite later if someone is motivated).

@eriktim
Copy link
Contributor Author

eriktim commented Jun 8, 2014

You're welcome, I am happy I can finally do something in return for this great library.

I am almost done extending from ol.format.TextFeature, but I do have a question about reading/writing multiple features: should the MULTI* geometries be split into mulitple features, or should this only be the case for GEOMETRYCOLLECTIONs?

P.S. I am willing to write a tokenizer based parser as well later on, if someone can provide me with an example to start from.

@tschaub
Copy link
Member

tschaub commented Jun 8, 2014

I think by default all WKT should be read as as a single geometry. For readFeatures the result would be an array of one feature. The GEOMETRYCOLLECTION would be read as ol.geom.GeometryCollection.

We could have an option to split GEOMETRYCOLLECTION into an array of features. This could look like the following:

var format = new ol.format.WKT({splitCollection: true});
var features = format.readFeatures(wktWithGeometryCollection);
// one feature for each geometry in the collection

I don't think this option should also split/explode MULTI* geometries. This could be handled by yet another option (splitMulti maybe).

@eriktim
Copy link
Contributor Author

eriktim commented Jun 8, 2014

Your solution makes sense, so I added the splitCollection option as well.
I'll push the new commits now, so let me know what you think.
By the way, is it required that readProjection is exported? For now I did not add the @todo api tag as it cannot return the projection anyway.

@eriktim
Copy link
Contributor Author

eriktim commented Jun 26, 2014

Any news on this?

@@ -31,7 +26,7 @@ var vector = new ol.layer.Vector({

var map = new ol.Map({
layers: [raster, vector],
renderer: ol.RendererHint.CANVAS,
renderer: exampleNS.getRendererFromQueryString(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the canvas renderer supports vector rendering. Can you remove this renderer option altogether ("canvas" is the default).

@tschaub
Copy link
Member

tschaub commented Jun 26, 2014

Thanks for the continued work on this @GingerIK. I made a few additional comments that should be straightforward to address.

At some point I imagine we could rewrite this with a simple lexer (these regexes are starting to show their age), but I think this will make a useful addition.

@eriktim
Copy link
Contributor Author

eriktim commented Jun 27, 2014

Thanks for your feedback! I just incorporated your comments and pushed a new commit.

When I find myself some extra time, I'll start trying to write a lexer if that's OK.

@@ -1,11 +1,9 @@
goog.require('ol.Feature');
goog.require('ol.Map');
goog.require('ol.RendererHint');
goog.require('ol.View2D');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that ol.View2D is gone, you'll want to change this to ol.View.

@tschaub
Copy link
Member

tschaub commented Jun 27, 2014

@GingerIK you'll want to rebase this on master.

git pull --rebase openlayers master
git push --force origin restore-wkt-parser

(This assumes you've named the remote pointing to https://github.com/openlayers/ol3/ openlayers and your own remote is origin).

If you're comfortable with git rebase, you could also squash some of those commits. I'm happy to help with this in a separate branch (using the commits you authored) if you could use a hand.

@eriktim
Copy link
Contributor Author

eriktim commented Jun 27, 2014

I'm comfortable with git rebase. Not with rewriting history on github, however ;-) But I think it worked.

@tschaub
Copy link
Member

tschaub commented Jun 27, 2014

Very nice work on this @GingerIK. Thanks for all the effort.

tschaub added a commit that referenced this pull request Jun 27, 2014
Back from the ashes!  The WKT format lives!
@tschaub tschaub merged commit 19cf117 into openlayers:master Jun 27, 2014
@eriktim
Copy link
Contributor Author

eriktim commented Jun 27, 2014

My pleasure.

@eriktim eriktim deleted the restore-wkt-parser branch June 27, 2014 21:59
@eriktim eriktim mentioned this pull request Jul 13, 2014
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