-
Notifications
You must be signed in to change notification settings - Fork 352
Add GeoJSON format support #1305
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 GeoJSON format support #1305
Conversation
f091d87
to
322db4e
Compare
322db4e
to
295d93d
Compare
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.
This is great to see - and a really nice approach to overlaying and generating it.
Can you explain your use case a bit? Are you planning to have multiple polygons with multiple colors? One potential issue I see is that multiple colored geojson data sets will require separate layers of images which could cause a bit of an explosion of memory.
One thought I had is that the geojson could be converted to SVG and then you'd have all the flexibility of SVG scalability and CSS styling. This geojson2svg package supports Geojson > SVG conversion. Who know's how the performance may be affected, though 🤔 Something to explore in the future.
edit And sorry for the commit rework. I wanted to remove the dependency on #1302 so we can get this merged separately if needed.
'https://www.gstatic.com/draco/v1/decoders/', | ||
); | ||
|
||
const geojson = { |
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.
What's the source for this polygon data?
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.
bacino_varenna_aggregato4326.json
I have this local file geojson (in order to load it here on Github, I had to change the extension from .geojson to .json), I wanted to try to have both an url or a geojson object.
the reasons for geojson object was because I wanted to start thinking about a "drawPlugin",
but also because maybe the front end developer could handle a conversion of it to the accepted EPSG,
in any case if just URL is provided, we should make sure that the geojson is defined in supported EPSG codes looking at 'crs.properties.name' of the object.
what do you think about this?
we can just keep the url if you prefer, to make it coherent with the library
example/geojson.js
Outdated
|
||
const geojsonOverlay = new GeoJSONTilesOverlay( { | ||
|
||
levels: 14, // choose max zoom to rasterize overlay ( 12- 14 typical ) |
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.
This is an interesting new case - this is a good solution for now. It may be worth adjusting overlays so that they can load up to whatever maximum resolution the underlying tile set is at in order to more easily support these kinds of dynamically generated / flexible types of data sets.
edit created #1310
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.
absolutely agree, I think that somehow we should maybe have access to the levels defined for the plugin the geojson is onto in order to manage that dynamically.
or hardcode an high number of levels if the prop is not provided?
In my use case, for the project I'm working on, I'd like the user to upload any geojson for a specific project (in supported EPSG, of course; the platform I'm working on could eventually convert them to the desired EPSG in the future), and these geojsons should be overlaid on a globe/basemap (photorealistic tiles, cesium globe, or whatever "base layer" the user wishes). One of the purposes of the geojson is to highlight, for example, the valley affected by a specific infrastructure (such as a bridge), so that the user can not only better see the area for whatever reason, but also understand, for example, whether a heavy rainfall (provided by a WMS) is within the area of interest or not, perhaps causing a flood, and so on.
that would be possible, I didn't think about it honestly, we could have colors for each feature, similarly to what is possible to do with your geojson library for three.js. about having multiple datasets, yes I understand that drawing on multiple canvas can be heavy if multiple data sets are there, in that case, I would consider, if possible, to have only one data set with multiple datasets, in order to create and draw using only one canvas per time, maybe having an Api from GeoJSONTilesOverlay/Plugin, to add a dataset and manage the "merge" of datasets behind the scene, or I would consider to test your suggestion about SVG. honestly I already made some tests with SVG but I have to dig more, I will consider that converter you linked me, or drawing them directly as I did in my last commit. I think that the advantage you see in using SVG is to don't have to worry about levels and redrawing images, in order to gain performance, am I right? I agree with that eventually 🚀 , but with current implementation svgs are created whenever it would create other images format, eventually we should keep track of the memory and cpu usage to see what is the best approach, and how to optimize it. |
As far as I understand, GeoJSON does not support custom CRS. The values should always be in latitude / longitude which will be universal. To that end the "GeoJSONImageSource" shouldn't need a custom CRS other than to adjust the implicit tiling scheme. Though I think that's probably not needed for this.
For reference it looks like Mapbox provides a custom styling system for things like this but it would be nice to avoid adding an overly complex styling system.
Unfortunately in order to render SVG in WebGL (or WebGPU afaik) you still have to rasterize the SVG data to a texture, so you will still have to deal with levels and the texture generation. But SVG is a good standard for these kinds of vector images and supports declarative styling out of the box rather than requiring a custom one. Once concern I have is that this will be slower, though, because of the complexity of the styles, but it will require some testing.
It looks like you've pulled and merged the cherry picked and rebased version of the branch I made into your local version, which has brought back all the styling changes I fixed. I can force push my version of the branch again but you'll need to pull it and overwrite your local version. Alternatively we can wait until #1302 is merged and you can fix up the styling issues in this branch then. The diff is otherwise too complex to understand now, unfortunately. Let me know what you'd like to do! |
Once it happened that I received a .geojson file in EPSG:3003, but that was in fact an exception, and not even the Cesium Viewer can render it by default, so I agree to treat them as latitude longitude coordinates, maybe we can just write a line in the docs or in a comment in the example about this
I agree, I would not add another 'rendering' layer, not for now at least. the more is simple the better it is imo.
that was in fact my doubt about webGL, but I agree that using SVG is better for drawing and styling, and can lead us to have a good and modular management of the styling eventually.
🙈 I am sorry I messed up with GH desktop. |
7ca1a3c
to
5d0586c
Compare
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.
Added some initial comments. I'll take a look at the drawing code later this week.
geojson = null, | ||
url = null, // URL or GeoJson object can be provided | ||
tileDimension = 256, | ||
bounds = [ - 180, - 90, 180, 90 ], |
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.
Instead of passing "bounds" into the image source it would be best to calculate the bounds of the geojson object so it's a tight bounds (including some margin to account for stroke thickness, etc.
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.
for the moment in this new commit I will set the bounds like for other image sources using the crs bounds, but if using the bounds limited to the entity can optimize the process, I would like to implement this, even in #1302 , a lot of WMS services like geojson use only a small portion of the map to display data in there.
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.
if using the bounds limited to the entity can optimize the process, I would like to implement this
Calling TilingScheme.setContentBounds
tells the tiling instance that any tiles that do not include that content bounds in its range is non existent and should not be loaded. This prevents unnecessary data loading and prevents render targets for overlays from being generated - saving memory and performance.
even in #1302 , a lot of WMS services like geojson use only a small portion of the map to display data in there.
I'd added this in some of the WMSImageSource updates - the constructor takes a contentBounds
field (which can be derived from the EX_GeographicBoundingBox in the WMS capabilities XML) and is used to set the content bounds of tiling here (with the full CRS range used as a fallback).
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.
this is awesome!! thank you for confirm me this
I tried setting the bounds, but seems that if I don't use very large offset, the geojson won't be even rendered, in the last commit I added a method to get the bounds from geojson, but it's usage is commented out as it's not working as expected, I would expect to don't even need to use an offset, but maybe I didn't get something about this
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.
It looks like there may actually be an issue with this that's appeared after some recent changes. I made #1320 to track it can can take care of it after this is merged.
const res = await this.fetchData( this.url ); | ||
// fetchData should return a Response-like object | ||
// allow both Response and plain object (in case overlay fetch returns parsed JSON) | ||
if ( res && typeof res.json === 'function' ) { | ||
|
||
this.geojson = await res.json(); | ||
|
||
} else if ( res ) { | ||
|
||
this.geojson = res; | ||
|
||
} |
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.
What case are you imagining where fetchData
will return pre-parsed JSON? I think it should be okay to assume this is performing a typical fetch in this case.
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 found out that it works parsing the data but also without parsing it, you can try i.e. with this public geojson used in 'url' parameter: italy municipalities
(it's a bit heavy as geojson, but I just used the first one I found)
but I think that the best thing to do is to parse it anyway, what do you think?
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'm not sure if I completely follow. Calling fetchData
is expected to return a Request
object, not a normal Javascript Object that could be GeoJSON - so the expectation is that we will always need to call "res.json()" here unless I'm missing something?
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.
now I got it, I am sorry I didn't understand what you mean (sometimes my english lacks), bear with me
that was a distraction error in the code, will be fixed!
} catch ( e ) { | ||
|
||
console.warn( 'GeoJSONImageSource: failed to fetch geojson from url', this.url, e ); | ||
// continue, but without geojson no tiles will be produced | ||
|
||
} |
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 think we should just let this init function throw an error so it can be handled elsewhere if needed. This is what the other image sources do.
…nor fixes and removed useless parameters
…son), fix for level generation and parsing. added also a commented parameter in the example, in order to test the system with a public geojson url
… gain performance, tested but not confirmed not completely successful, as it need a big offset in order to render the geojson
… made sure to use all "type" of geojson, canvas rounding to integer
Thanks for the updates! I'll plan to make a few changes to simplify the code in some places. For now I think we can remove the SVG path, as well. We can always come back to this PR to get it in the history if needed. |
# Conflicts: # src/three/plugins/images/EPSGTilesPlugin.js # src/three/plugins/images/ImageOverlayPlugin.d.ts # src/three/plugins/images/ImageOverlayPlugin.js
Thanks! I'm going to merge this and move it into the Thank you again for all the work on this. |
thanks to you for all the support and the help! |
added geojson support as overlay and tilesplugin