-
Notifications
You must be signed in to change notification settings - Fork 303
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
feat(parser): add a shapefile parser #1056
Conversation
35bab6c
to
5c599bf
Compare
This PR can be reviewed now that #1033 has been merged. |
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.
could you move data files in itowns sample data?
examples/shapefile.html
Outdated
|
||
// Load all the necessary files for a shapefile, parse them and | ||
// display them. | ||
Promise.all([ |
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.
could you add Fetcher.Shape(baseUrl, extentionfiles = {shp: 'shp',.... }, networkOptions)
?
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.
So I pushed a quick draft, let me know what you think
951ef5d
to
c0df145
Compare
I completed the documentation and rebased everything in 2 commits |
I have some comments:
|
|
src/Converter/Feature2Texture.js
Outdated
function drawFeature(ctx, feature, origin, scale, extent, style = {}) { | ||
let gStyle = style; | ||
// Determinate with radius point | ||
const px = 0.01; |
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.
px
needs to be determinate with radius, because if radius increases then the condition is wrong.
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.
Damn I copied/pasted it without noticing the comment, my bad ! Fixing it.
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.
So this seems complicated to do: it depends a lot on the current projection/view, and I don't think this will simply things. What about allowing only a maximum radius size ? I don't see a case where a point is larger than 30 or 50px, and unless someone displays this point on a globe with a zoom at 0, there will be no artefact. What do you say ?
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.
Nervemind, it is done simply in fact, my bad
fixed with #1068 |
The parser can take into account four files: shp, shx, dbf and prj. It uses https://www.npmjs.com/package/shpjs An example and some data has been added.
The new multiple method can be used to load a bunch of files having the same base url, like the shapefile system for example.
|
||
// cross multiplication to know in the extent system the real size of | ||
// the point | ||
px = gStyle.radius * (extentDim.x / 256); |
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.
you could use ctx.canvas.width
instead of 256
The parser can take into account four files: shp, shx, dbf and prj. It
uses https://www.npmjs.com/package/shpjs
An example and some data has been added.
This PR should not be merged before #1033