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

Raster Tiles #205

Closed
wants to merge 22 commits into from
Closed

Raster Tiles #205

wants to merge 22 commits into from

Conversation

quillasp
Copy link

@quillasp quillasp commented Nov 11, 2022

Associated with the issue #71.

This draft PR can load and display the raster tiles. It still needs a way to do different requests based on the tile type (vector or raster for the moment).

💻 Examples

🚨 Test instructions

✔️ PR Todo

  • Can do both types of requests
  • Raster data loading and storing
  • Raster rendering

@maxammann
Copy link
Collaborator

Just tried to run it on Linux. It compiles and runs, but its glitching quite a bit.
Do you think that is a bug with my environment or another issue?

@quillasp
Copy link
Author

Just tried to run it on Linux. It compiles and runs, but its glitching quite a bit. Do you think that is a bug with my environment or another issue?

I don't have a Linux machine so I can't test right away but it shouldn't be different from other platforms. 🤔

@maxammann maxammann changed the title Rebase with the new codebase Raster Tiles Nov 14, 2022
@maxammann maxammann mentioned this pull request Nov 14, 2022
3 tasks
Still finding a way to figure out different requests depending on the tile type inside the request stage
@quillasp quillasp force-pushed the ft-branch-raster-tiles branch 4 times, most recently from 2f87731 to 7ed0914 Compare November 23, 2022 02:33
Use of a projection matrix from one space to another, the tiles are now square.
Fix raster

Fix raster
@quillasp quillasp force-pushed the ft-branch-raster-tiles branch from 7ed0914 to 104ff8a Compare November 23, 2022 03:04
@quillasp quillasp marked this pull request as ready for review November 23, 2022 16:01
@maxammann
Copy link
Collaborator

@quillasp web is failing. We probably miss some more feature flags

Copy link
Collaborator

@maxammann maxammann left a comment

Choose a reason for hiding this comment

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

Looks very cool already! I mean yeah there are a few rough edges, but that is also due to some architectural changes we need to do in order to add the feature.

maplibre/Cargo.toml Outdated Show resolved Hide resolved
@@ -85,6 +86,9 @@ smallvec = "1.9.0"

# Headless
png = { version = "0.17.5", optional = true }
image = { version = "0.24", default-features = false, features = ["jpeg", "webp"], optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we might want png here aswell. Also with image we probably can remove the png crate

// Used so the API key is not in the source code.
// Put an '.env' file in the root directory with the following content:
// MAPTILER_API_KEY=your_api_key
let api_key = env::var("MAPTILER_API_KEY").expect("MAPTILER_API_KEY must be set");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't work on e.g. web. Fine for now though.

We should just create a task.

Comment on lines 134 to 151
if let TileType::Vector(vector_tile) = &tile {
let available_layers: HashSet<_> = vector_tile
.layers
.iter()
.map(|layer| layer.name.clone())
.collect::<HashSet<_>>();

for missing_layer in tile_request.layers.difference(&available_layers) {
context
.processor_mut()
.layer_unavailable(coords, missing_layer)?;

tracing::info!(
"requested layer {} at {} not found in tile",
missing_layer,
&coords
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of conditionally doing this we should probably create two separate pipeline steps.

Raster(Vec<u8>),
}

impl From<TileType> for geozero::mvt::Tile {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Implement TryForm instead of From

Comment on lines 7 to 23
fn main(
@location(0) position: vec2<f32>,
@location(1) tex_coords: vec2<f32>,
@location(4) translate1: vec4<f32>,
@location(5) translate2: vec4<f32>,
@location(6) translate3: vec4<f32>,
@location(7) translate4: vec4<f32>,
@location(9) zoom_factor: f32,
) -> VertexOutput {
let z = 0.0;
let width = 3.0 * zoom_factor;
let normal = vec2<f32>(0.0,0.0);

var position = mat4x4<f32>(translate1, translate2, translate3, translate4) * vec4<f32>(position + normal * width, z, 1.0);
position.z = 1.0;

return VertexOutput(tex_coords, position);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe some stuff is duplicated here. Fine for now though!

raster_resources
.texture
.as_ref()
.unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If possible remove that unwrap

@@ -223,6 +238,66 @@ impl UploadStage {
&feature_metadata,
);
}
StoredLayer::RasterLayer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would favor to create a new stage: upload_raster_stage and then conditionally add stat stage, instead of having the inline cfg flag

@@ -84,6 +87,25 @@ impl RenderPipeline for TilePipeline {
},
count: None,
}]])
} else if self.raster {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure yet how we can improve this. This is very specific for a file which tries to keep generic.

maplibre/src/render/stages/upload_stage.rs Outdated Show resolved Hide resolved
Comment on lines 86 to 89
#[cfg(feature = "raster")]
let source = SourceType::Raster(RasterSource::default());
#[cfg(not(feature = "raster"))]
let source = SourceType::Tessellate(TessellateSource::default());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does that mean if raster is enabled then vector tiles do no longer work?

@maxammann
Copy link
Collaborator

I introduced quite a lot of merge conflicts. I will resolve them today!

# Conflicts:
#	maplibre/src/error.rs
#	maplibre/src/headless/map.rs
#	maplibre/src/io/pipeline.rs
#	maplibre/src/io/source_client.rs
#	maplibre/src/io/tile_pipelines.rs
#	maplibre/src/io/transferables.rs
#	maplibre/src/stages/populate_tile_store_stage.rs
#	maplibre/src/stages/request_stage.rs
#	web/src/platform/singlethreaded/apc.rs
#	web/src/platform/singlethreaded/transferables.rs
@maxammann
Copy link
Collaborator

Done, I also simplified the pipeline slightly and updated the APC implementation for Web.

# Conflicts:
#	maplibre/src/io/tile_pipelines.rs
#	maplibre/src/render/render_commands.rs
#	maplibre/src/render/stages/resource_stage.rs
#	maplibre/src/render/stages/upload_stage.rs
#	maplibre/src/render/tile_pipeline.rs
#	maplibre/src/stages/request_stage.rs
@maxammann
Copy link
Collaborator

@quillasp I pushed a simplification which does not require the BufferPool

@maxammann
Copy link
Collaborator

Merged in #253

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.

3 participants