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

Expose valueToColor4 as constructor option #393

Closed
chrissantamaria opened this issue Oct 25, 2022 · 3 comments · Fixed by #399
Closed

Expose valueToColor4 as constructor option #393

chrissantamaria opened this issue Oct 25, 2022 · 3 comments · Fixed by #399

Comments

@chrissantamaria
Copy link
Contributor

👋 this package has been very handy! Was looking to write a custom valueToColor4 function as mentioned in the README, though it looks like this isn't customizable via the constructor options (though valueToColor is). Is there any reason this can't / shouldn't be updated?

If not, I'm happy to open a PR and update this. Thanks!

@vinayakkulkarni
Copy link
Owner

👋 this package has been very handy! Was looking to write a custom valueToColor4 function as mentioned in the README, though it looks like this isn't customizable via the constructor options (though valueToColor is). Is there any reason this can't / shouldn't be updated?

If not, I'm happy to open a PR and update this. Thanks!

This is already exposed – https://github.com/vinayakkulkarni/mapbox-gl-interpolate-heatmap/blob/main/src/layer.ts#L20-L24

@chrissantamaria
Copy link
Contributor Author

Perhaps I'm misunderstanding - the constructor type seems to accept valueToColor4 but doesn't actually use it to update the default class field:

constructor(options: HeatmapLayer) {
this.id = options.id || '';
this.data = options.data || [];
this.aoi = options.aoi || [];
this.valueToColor =
options.valueToColor ||
`
vec3 valueToColor(float value) {
return vec3(max((value-0.5)*2.0, 0.0), 1.0 - 2.0*abs(value - 0.5), max((0.5-value)*2.0, 0.0));
}
`;
this.opacity = options.opacity || 0.5;
this.minValue = options.minValue || Infinity;
this.maxValue = options.maxValue || -Infinity;
this.p = options.p || 3;
this.framebufferFactor = options.framebufferFactor || 0.3;
// Having a framebufferFactor < 1 and a texture that don't cover the entire map results in visual artifacts, so we prevent this situation
this.textureCoverSameAreaAsROI = this.framebufferFactor === 1;
}

I think this would just require a small addition to the constructor:

  constructor(options: HeatmapLayer) {
    this.id = options.id || '';
    this.data = options.data || [];
    this.aoi = options.aoi || [];
    this.valueToColor =
      options.valueToColor ||
      `
      vec3 valueToColor(float value) {
          return vec3(max((value-0.5)*2.0, 0.0), 1.0 - 2.0*abs(value - 0.5), max((0.5-value)*2.0, 0.0));
      }
  `;
+    this.valueToColor4 =
+      options.valueToColor4 ||
+      `
+      vec4 valueToColor4(float value, float defaultOpacity) {
+          return vec4(valueToColor(value), defaultOpacity);
+      }
+  `;
    this.opacity = options.opacity || 0.5;
    this.minValue = options.minValue || Infinity;
    this.maxValue = options.maxValue || -Infinity;
    this.p = options.p || 3;
    this.framebufferFactor = options.framebufferFactor || 0.3;
    // Having a framebufferFactor < 1 and a texture that don't cover the entire map results in visual artifacts, so we prevent this situation
    this.textureCoverSameAreaAsROI = this.framebufferFactor === 1;
  }

@vinayakkulkarni
Copy link
Owner

Perhaps I'm misunderstanding - the constructor type seems to accept valueToColor4 but doesn't actually use it to update the default class field:

constructor(options: HeatmapLayer) {
this.id = options.id || '';
this.data = options.data || [];
this.aoi = options.aoi || [];
this.valueToColor =
options.valueToColor ||
`
vec3 valueToColor(float value) {
return vec3(max((value-0.5)*2.0, 0.0), 1.0 - 2.0*abs(value - 0.5), max((0.5-value)*2.0, 0.0));
}
`;
this.opacity = options.opacity || 0.5;
this.minValue = options.minValue || Infinity;
this.maxValue = options.maxValue || -Infinity;
this.p = options.p || 3;
this.framebufferFactor = options.framebufferFactor || 0.3;
// Having a framebufferFactor < 1 and a texture that don't cover the entire map results in visual artifacts, so we prevent this situation
this.textureCoverSameAreaAsROI = this.framebufferFactor === 1;
}

I think this would just require a small addition to the constructor:

  constructor(options: HeatmapLayer) {

    this.id = options.id || '';

    this.data = options.data || [];

    this.aoi = options.aoi || [];

    this.valueToColor =

      options.valueToColor ||

      `

      vec3 valueToColor(float value) {

          return vec3(max((value-0.5)*2.0, 0.0), 1.0 - 2.0*abs(value - 0.5), max((0.5-value)*2.0, 0.0));

      }

  `;

+    this.valueToColor4 =

+      options.valueToColor4 ||

+      `

+      vec4 valueToColor4(float value, float defaultOpacity) {

+          return vec4(valueToColor(value), defaultOpacity);

+      }

+  `;

    this.opacity = options.opacity || 0.5;

    this.minValue = options.minValue || Infinity;

    this.maxValue = options.maxValue || -Infinity;

    this.p = options.p || 3;

    this.framebufferFactor = options.framebufferFactor || 0.3;

    // Having a framebufferFactor < 1 and a texture that don't cover the entire map results in visual artifacts, so we prevent this situation

    this.textureCoverSameAreaAsROI = this.framebufferFactor === 1;

  }

Feel free to open an PR ;)

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 a pull request may close this issue.

2 participants