feat(shader_ui_controls): add select control

[chrisj]

Related to #825

This PR adds dropdown support to shader UI controls.

Users can now define a dropdown in shader code with a #uicontrol directive, for example:

#uicontrol uint myMode dropdown(options=["one", "two", "three"], default=1)

The value of the dropdown is stored in the state and accessed in the shader as the index

Example

#uicontrol uint mode dropdown(options=["large", "small", "default"], default=2)
void main() {
  if (mode == 0u) {
    setPointMarkerSize(50.0);
  } else if (mode == 1u) {
    setPointMarkerSize(2.0);
  }
  setColor(defaultColor());
}

https://cj-shader-dropdown-share-dot-neuroglancer-dot-seung-lab.appspot.com/#!%7B%22dimensions%22:%7B%22x%22:%5B1.8e-8%2C%22m%22%5D%2C%22y%22:%5B1.8e-8%2C%22m%22%5D%2C%22z%22:%5B4.5e-8%2C%22m%22%5D%7D%2C%22position%22:%5B49167.62890625%2C49151.15234375%2C1694.5%5D%2C%22crossSectionScale%22:0.5856692901447939%2C%22projectionScale%22:131072%2C%22layers%22:%5B%7B%22type%22:%22annotation%22%2C%22source%22:%7B%22url%22:%22local://annotations%22%2C%22transform%22:%7B%22outputDimensions%22:%7B%22x%22:%5B1.8e-8%2C%22m%22%5D%2C%22y%22:%5B1.8e-8%2C%22m%22%5D%2C%22z%22:%5B4.5e-8%2C%22m%22%5D%7D%7D%7D%2C%22tool%22:%22annotatePoint%22%2C%22tab%22:%22rendering%22%2C%22annotations%22:%5B%7B%22point%22:%5B49149.74609375%2C49037.05078125%2C1694.5%5D%2C%22type%22:%22point%22%2C%22id%22:%2279a0f82872520aa6471d37c0589fcff1931dc7da%22%7D%2C%7B%22point%22:%5B49220.8046875%2C49090.0859375%2C1694.5%5D%2C%22type%22:%22point%22%2C%22id%22:%22f5fda6876e7ddb4d49d5926e869c0483b615e15b%22%7D%2C%7B%22point%22:%5B49268.84765625%2C49172.140625%2C1694.5%5D%2C%22type%22:%22point%22%2C%22id%22:%22930b95f8c109ee320ef875551cf024d957f9e90d%22%7D%2C%7B%22point%22:%5B49172.765625%2C49222.171875%2C1694.5%5D%2C%22type%22:%22point%22%2C%22id%22:%225b152a16ddf37e05a1ddd777c88cea0973cd4a06%22%7D%2C%7B%22point%22:%5B49079.69140625%2C49161.1328125%2C1694.5%5D%2C%22type%22:%22point%22%2C%22id%22:%2280623dbccc502080b45f446b5ffaeb22eebca051%22%7D%2C%7B%22point%22:%5B49066.6796875%2C49083.078125%2C1694.5%5D%2C%22type%22:%22point%22%2C%22id%22:%220071719479ad78f5de3299afe8bf44b1573df4e3%22%7D%5D%2C%22shader%22:%22#uicontrol%20uint%20mode%20dropdown%28options=%5B%5C%22large%5C%22%2C%20%5C%22small%5C%22%2C%20%5C%22default%5C%22%5D%2C%20default=2%29%5Cnvoid%20main%28%29%20%7B%5Cn%20%20if%20%28mode%20==%200u%29%20%7B%5Cn%20%20%20%20setPointMarkerSize%2850.0%29%3B%5Cn%20%20%7D%20else%20if%20%28mode%20==%201u%29%20%7B%5Cn%20%20%20%20setPointMarkerSize%282.0%29%3B%5Cn%20%20%7D%5Cn%20%20setColor%28defaultColor%28%29%29%3B%5Cn%7D%5Cn%22%2C%22name%22:%22annotation%22%7D%5D%2C%22selectedLayer%22:%7B%22size%22:568%2C%22visible%22:true%2C%22layer%22:%22annotation%22%7D%2C%22layout%22:%22xy%22%2C%22selection%22:%7B%22size%22:568%2C%22visible%22:false%7D%7D

[fcollman]

Just to add some enthusiasm for this, this would allow users to build shaders that have functionalities with options/modes, but hide that complexity from the user through picking amongst these dropdown options.

@stuarteberg I think this addresses your feature request.

[jbms]

Looks good.

A few comments:

• The array of strings representation looks good but it could be nice to also support a json object, {0: "large", 1: "small", 2: "default"}
• The default value should be able to be specified as a string
• Maybe it should be called select or enum rather than dropdown which is kind of generic?
• Are there ways to unify this with existing and in-progress support for enum / string properties?

[chrisj]

@jbms should the value still be stored in the state as an index or as a string? And do you mean supporting both index and string for the default value? I see the benefit of the json object in that you can match the index you define in options to the one used in the user shader code.

I could unify this though I'm not sure it would be beneficial the implementation overlap is small, I've been using property for the seg prop control. The easiest would be something like select(type="segmentProperty")

[jbms]

I don't have a strong opinion about how it is encoded in the state --- at least it should accept both on input, I'd say.

Yes, default value should support both string and index.

As far as unify with the in-progress support for enum / string properties, I was thinking along the lines of allowing you to specify the names directly in the shader and avoid having to use the indices at all.

[chrisj]

#uicontrol uint mode select(options=["large", "small", "default"], default="small")
void main() {
  if (mode == "large") {
    setPointMarkerSize(50.0);
  } else if (mode == "small") {
    setPointMarkerSize(2.0);
  }
  setColor(defaultColor());
}

Renamed dropdown to select.

I added support for raw strings in the shader. To keep this feature simple, I removed the ability to set a specific integer value for each select option.

Each unique string is converted to an corresponding unsigned integer value. This would also support segment properties.

[lanesawyer]

Looking good! One request and one question, but I tested this by playing with the shader and it's working well.

I'm not going to be approving or requesting changes on PRs at this point, just here to start wrapping my head around the code by looking at some PRs!

[lanesawyer]

Could you pull this out into a variable with a good name to give a little bit more context on what the regex is doing?

[lanesawyer]

New tests look solid!

[lanesawyer]

I'm not a fan of the non-null assertion operator since it can lead to bugs down the line if things are refactored poorly, but is that commonly used in this codebase?

Not asking for a change, just trying to understand the practices here 😄

[chrisj]

thanks for the feedback!

Sorry I skipped your question. Non-null assertion is accepted and used sporadically, I would say primarily in cases where typescript can't follow what is otherwise clearly correct.

There is definitely some usage in neuroglancer that could be cleaned up, here for instance

neuroglancer/src/chunk_manager/backend.ts

Lines 362 to 365 in f88c50f

	if (!this.listeners_.has(key)) {
	return false;
	}
	const keyListeners = this.listeners_.get(key)!;

My feeling is we would prefer to avoid it unless the alternative is harder to follow.

[seankmartin]

generally looks good! Only comment I have is that the tool which shows on keypress related to the select is not very easy to use. I can't seem to manually change the dropdown by clicking, and there is no bind like shift+wheel or similar to move through the options from the tool