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

Feature/auto color #84

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Feature/auto color #84

wants to merge 2 commits into from

Conversation

kciurleo
Copy link
Contributor

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.)

Copy link
Contributor

@darrenhunt2 darrenhunt2 left a 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?

@kciurleo
Copy link
Contributor Author

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 - camera_can_bin being false will result in that dropdown not being available (which is the case for ECO) separate from the auto color options

Copy link
Collaborator

@timbeccue timbeccue left a 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,
Copy link
Collaborator

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>
Copy link
Collaborator

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"
Copy link
Collaborator

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
Copy link
Collaborator

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 [[]]
Copy link
Collaborator

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 [[]].

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.

3 participants