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

Added raster-(hue-rotate/contrast/saturation/opacity/brightness-*/) as operations #1055

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

orangemug
Copy link
Contributor

@orangemug orangemug commented Dec 12, 2023

This PR adds the following styling rules

Missing: Tests for new raster-* operations.

if (!id || id != glSourceId) {
if (
// This line is because rasters set properties on their source
glLayer.type === 'raster' ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the line I'm most unsure about. We need this because is we have two layers with the same source they need to apply different operations to their sources.

@orangemug orangemug changed the title Added raster-(hue-rotate/saturation/opacity) as operations Added raster-(hue-rotate/opacity) as operations Dec 14, 2023
@orangemug
Copy link
Contributor Author

Tests are failing due to the addition of RasterSource, if y'all are happy with the approach, I'll work on fixing them. Let me know.

@ahocevar
Copy link
Member

I'm sorry, but I won't be able to review this for a while. I'm terribly busy at the moment.

@orangemug
Copy link
Contributor Author

No worries @ahocevar

@orangemug orangemug changed the title Added raster-(hue-rotate/opacity) as operations Added raster-(hue-rotate/hue-contrast/hue-saturation/opacity/brightness-*/) as operations Dec 18, 2023
@orangemug orangemug changed the title Added raster-(hue-rotate/hue-contrast/hue-saturation/opacity/brightness-*/) as operations Added raster-(hue-rotate/contrast/saturation/opacity/brightness-*/) as operations Dec 18, 2023
@orangemug
Copy link
Contributor Author

Tests are failing due to the addition of RasterSource, if y'all are happy with the approach, I'll work on fixing them. Let me know.

Updated this to support more of the spec, if you are happy I'll fix up the tests.

@ahocevar
Copy link
Member

Tests are failing due to the addition of RasterSource, if y'all are happy with the approach, I'll work on fixing them. Let me know.

I'm failing to understand why existing tests are failing - RasterSource was already there before. At the moment I don't have time to give it a closer look, but if you are able to add a commit that helps understand what you think is wrong with the tests, it would help.

@orangemug
Copy link
Contributor Author

I'm failing to understand why existing tests are failing

@ahocevar I'm still doing some hunting on this one, hence still in draft. Essentially previously a raster was a

TileLayer with a TileJSON source

Now it's a

ImageLayer with a Raster source. The Raster source has the sources array set to a TileLayer with a TileJSON source.

So essentially there is this new wrapper, where tileLayer below is the same as it was previously.

  const layer = new ImageLayer({
    source: new Raster({
      operationType: 'image',
      operation: rasterShader,
      sources: [tileLayer],
    }),
  });

So the source in the following line of the tests now refers to the Raster rather than TileJSON source.

      it('creates the correct tile grid for raster sources', function (done) {
        apply(target, context)
          .then(function (map) {
            const statesSource = map.getLayers().item(0).getSource();  // <======== HERE
            const statesTileGrid = statesSource.getTileGrid();
            should(statesTileGrid.getTileSize()).eql(256);
            should(statesTileGrid.getExtent()).eql([
              -20037508.342789244, -20037508.342789244, 20037508.342789244,
              20037508.342789244,
            ]);

Seems like an easy fix right? But I can't appear to find the openlayers API to get to the tileJson (shown below) once the layer is instantiated

  const layer = new ImageLayer({
    source: new Raster({
      operationType: 'image',
      operation: rasterShader,
      sources: [tileLayer],    // <=== Not sure how to get at this from the test suite.
    }),
  });

I'm happy doing some more hunting, but it'll probably be a couple of days, as I'm a little busy right now.

@ahocevar
Copy link
Member

If I understand you correctly, a raster layer is now always an Image layer with a raster source? If so, it's not a good idea. Fine if any raster transformations that require a raster source are applied, bit without that, we should continue to use the Tile layer with the TileJSON source. Simply because the Raster source adds a significant performance penalty.

@orangemug
Copy link
Contributor Author

If I understand you correctly, a raster layer is now always an Image layer with a raster source? If so, it's not a good idea. Fine if any raster transformations that require a raster source are applied, bit without that, we should continue to use the Tile layer with the TileJSON source. Simply because the Raster source adds a significant performance penalty.

Yeah that makes sense, I'll add a branch for raster without operations. I'll try and add some new tests for the raster operations as well.

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.

2 participants