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

Tracker: User Feedback #577

Closed
3 of 23 tasks
ibgreen opened this issue Apr 17, 2017 · 9 comments
Closed
3 of 23 tasks

Tracker: User Feedback #577

ibgreen opened this issue Apr 17, 2017 · 9 comments
Labels
Milestone

Comments

@ibgreen
Copy link
Collaborator

ibgreen commented Apr 17, 2017

A list of weaknesses in v4 docs and API based on questions and issues reported by our users. Can be addressed through API changes, better docs, FAQ articles etc.

V5/V6 API

V4 API

ONGOING

UNDER CONSIDERATION

UNDER CONSIDERATION

ALL REPOS

OTHER REPOS

OTHER

@ibgreen ibgreen added this to the v4.1 milestone May 4, 2017
@jefffriesen
Copy link

I have one suggestion on documentation for updateTriggers. Right now the docs say this:

This prop expects an object of which the keys matching the accessor names of a layer.
If any values in this object are changed between props update, the attribute corresponding
to the accessors named by the key will be invalidated.

I'm not clear what that means. I think what's missing isn't what the keys should be, it's what the values for the keys should be. I've looked through the repo and sometimes there are functions, sometimes values. Sometimes the original values don't match the value in updateTriggers. For example:

const MALE_COLOR = [0, 128, 255]
const FEMALE_COLOR = [255, 0, 128]

// later in the code...
    const layer = new ScatterplotLayer({
      id: 'scatter-plot',
      data,
      radiusScale: radius,
      radiusMinPixels: 0.25,
      getPosition: d => [d[0], d[1], 0],
      getColor: d => d[2] === 1 ? maleColor : femaleColor,
      getRadius: d => 1,
      updateTriggers: {
        getColor: {c1: maleColor, c2: femaleColor}
      }
    });

In the above case, the primary getColor returns an array of numbers. The getColor key inside updateTriggers returns an object. Not sure what c1 and c2 are in this case.

In other places, we have 2 different functions for getPosition:

      new PlotLayer({
        getPosition: (u, v) => {
          const x = (u - 1 / 2) * Math.PI * 2;
          const y = (v - 1 / 2) * Math.PI * 2;
          return [x, y, equation(x, y)];
        },
        getColor: (x, y, z) => [40, z * 128 + 128, 160],
        getXScale: getScale,
        getYScale: getScale,
        getZScale: getScale,
        uCount: resolution,
        vCount: resolution,
        drawAxes: showAxis,
        axesPadding: 0.25,
        axesColor: [0, 0, 0, 128],
        opacity: 1,
        pickable: Boolean(this.props.onHover),
        onHover: this.props.onHover,
        updateTriggers: {
          getPosition: equation
        }
      })

// where equation is:
const equation = (x, y) => {
  return Math.sin(x * x + y * y) * x / Math.PI;
};

I have a work around for this so I'm not blocked, but I think this would be important for updated documentations. updateTriggers is used in lots of examples in the repo, which indicates to me it's important (and I could use it if I understood it).

Thanks

@Pessimistress
Copy link
Collaborator

Pessimistress commented Jul 13, 2017

@jefffriesen Thanks for the feedback! I agree that we're not doing a good job explaining updateTriggers.
To unblock you, let me try to explain it here.

  • Accessors such as getColor and getPosition are called to retrieve colors and positions when you first add the layer. From then on, to maximize performance, deck.gl does not recalculate colors or positions unless the data prop changes.
  • Sometimes data remains the same, but the outcome of an accessor has changed. In the scatterplot example, this is caused by the user interacting with the color input. In this case, you need to explicitly inform deck.gl to re-evaluate getColor for all data items.
  • You do so by defining updateTriggers. The following code:
    updateTriggers: {
        getColor: {c1: maleColor, c2: femaleColor}
    }

Says that the output of getColor is affected by two values c1 and c2. Every render cycle, deck.gl compares updateTriggers.getColor with its previous value 1-level deep (this is so that you can specify multiple dependencies). If either c1 or c2 changes, any attribute that depends on getColor will be updated.

  • The "trigger" value may be a string, object or function - as long as it can be shallow-compared with the previous value.

Let me know if this helps. If so, I will add a page to documentation on this topic.

@jefffriesen
Copy link

@Pessimistress Thanks so much for helping me out on this. Some of the explanation above makes sense to me, some not.

Accessors such as getColor and getPosition are called to retrieve colors and positions when you first add the layer. From then on, to maximize performance, deck.gl does not recalculate colors or positions unless the data prop changes.

Perfectly clear and well designed

Sometimes data remains the same, but the outcome of an accessor has changed. In the scatterplot example, this is caused by the user interacting with the color input. In this case, you need to explicitly inform deck.gl to re-evaluate getColor for all data items.

Yep, someone types in a new maleColor/femaleColor value in the input box and now

getColor: d => d[2] === 1 ? maleColor : femaleColor,` 

has a different result, even though d is the same.

Says that the output of getColor is affected by two values c1 and c2.

Where did c1 and c2 come from? Are they arbitrarily-named keys? They are not in the manhattan.json data structure (which is an array of arrays). Is there some meaning with the key names or could I just call them {a: maleColor, b: femaleColor}? If that's the case, why not pass in an array like [maleColor, femaleColor]?

Every render cycle, deck.gl compares updateTriggers.getColor with its previous value 1-level deep (this is so that you can specify multiple dependencies).

It will only look 1 level deep even with nested data structures, even though in this example, getColor is returning a simple array. But I'm not clear on the multiple dependencies aspect. Multiple dependencies for what?

The "trigger" value may be a string, object or function - as long as it can be shallow-compared with the previous value.

This statement is close to helping, but I can't reconcile it with the c1 and c2 object above.

Hopefully going through and explaining this to me like I'm 5 will help out others when reading the documentation! Thanks again.

@howtimeflies0
Copy link

@jefffriesen thanks a lot for asking for clarification. This will greatly help the quality of deckgl's documentation. Please send similar feedback here or create separate issues on github when you find more unclear docs!

@Pessimistress your explanation is super clear. We should definitely put your explanation to our docs!

@Pessimistress
Copy link
Collaborator

@jefffriesen Think of updateTriggers.getColor as a list of dependencies that affect getColor's output - it can be {c1: maleColor, c2: femaleColor}, or {maleColor, femaleColor}, or [maleColor, femaleColor]. In fact, I like your suggestion. I think the arbitrary keys c1 c2 really threw you off there. I can update the example code.

@jefffriesen
Copy link

@Pessimistress Oh, that clears things up. I forked the repo and was looking at the code, but I guess in retrospect I should have run the example and tried different things.

I wonder if having that much flexibility in specifying update triggers is more confusing. When I was searching the code for examples, I saw several different ways of doing it. To me that was more confusing because I was trying to pick up on the pattern, whereas if it was consistent the pattern would be easier to spot. Is there a reason not to just have an array of values instead of objects with arbitrary keys?

@ericsoco
Copy link
Contributor

@jefffriesen you've probably already discovered this, but this is the updateTriggers diff implementation, and it supports a single value, an array of values, or a key-value map (object). We're opting to maintain flexibility but the above PR limits the documented usage to just a value or array of values.

Thanks for the feedback!

@ibgreen ibgreen modified the milestones: v.Next, v4.1 Aug 15, 2017
@ibgreen ibgreen changed the title v4 Feedback Tracker v4.2 Feedback Tracker Aug 18, 2017
@howtimeflies0
Copy link

I plan to create issues for each item in the list and use our newly created workboard to track them.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Aug 28, 2017

I plan to create issues for each item in the list and use our newly created workboard to track them.

@shaojingli Sounds good but maybe we should go through the status first, these were created a while ago.

@1chandu 1chandu removed this from the v4.2 milestone Oct 24, 2017
@ibgreen ibgreen added this to the v4.2 milestone Nov 13, 2017
@1chandu 1chandu modified the milestone: v4.2 Nov 14, 2017
@1chandu 1chandu changed the title v4.2 Feedback Tracker v5.0 Feedback Tracker Dec 13, 2017
@1chandu 1chandu modified the milestones: v5.0, v6.0 Dec 19, 2017
@ibgreen ibgreen changed the title v5.0 Feedback Tracker User Feedback (Multi-Release Tracker) Jul 31, 2018
@ibgreen ibgreen changed the title User Feedback (Multi-Release Tracker) Tracker: User Feedback Dec 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants