-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/auto color #84
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me right now, and the auto-color dropdown on sites that currently support it looks good! Just to double-check, is it meant to replace the Resolution dropdown (which it appeared to while testing on ECO)? Or is that just something to do with that site's config specifically?
That has to do with that site's config specifically - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor changes, but the functionality seems to work fine!
@@ -431,6 +433,7 @@ export const commands_mixin = { | |||
count: this.camera_count.toString(), | |||
bin: JSON.stringify(this.camera_bin), | |||
filter: this.filter_wheel_options_selection, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might make more sense to not include the filter if auto_color is set to something besides manual. It maps to reality a little better, but it does make the code a little more complicated. So I'm not 100% sold in either direction. Thoughts?
horizontal | ||
label="Auto-Color" | ||
> | ||
<b-field> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this tag necessary?
v-for="(color_opt, index) in auto_color_options" | ||
:key="index" | ||
:value="color_opt" | ||
:selected="index === 0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary because the default selection is already defined through the v-model setting.
@@ -187,6 +187,13 @@ const getters = { | |||
return fwo | |||
}, | |||
|
|||
// Available automatic 3-color image options | |||
auto_color_options: (state, getters) => { | |||
const fwo = getters.selected_filter_wheel_config.settings?.auto_color_options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwo (from the method above) refers to filter wheel options (admittedly not a fantastic name). I think a different name here would help future readability.
// Available automatic 3-color image options | ||
auto_color_options: (state, getters) => { | ||
const fwo = getters.selected_filter_wheel_config.settings?.auto_color_options | ||
if (fwo == undefined) return [[]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally a good idea to use the strict equality operator ===
to check if something is undefined. The loose equality check ==
will equate undefined
and null
, which usually isn't intended behavior. (I know, my mistake in the method above too.)
Also, while the filter wheel options expects a different format (the first array element is an array of keys, so an empty value still needs one element). But the auto_color_options is just a simple array of values. So the empty value should just be a simple []
rather than [[]]
.
This pr adds the ability to request automatic 3-color images from the frontend. Options for a site should be defined as an array in the filter wheel config settings named
auto_color_options
, with "manual" as one of them (meaning no auto color should be performed). If there are more options than just "manual", an auto color selection will be added to the camera controls. When the selection is changed from its default of "manual," filter selection is disabled.(Scaling of exposure time/filter choice/etc is handled in the obsy code once the command is received.)