Support image annotation text file (backend)

[pford]

Supported in v1.4:

• name
• color
• line width
• dashed line

Requirements for v4:
https://docs.google.com/document/d/1YCB_lIVH8ftJcNS0gliE2uHmuiowGkV6DQYJlfteJUM/edit#

Must support:

• SetRegion
• ExportRegion
• ImportRegion
• ResumeSession
• Import/Export from matched image

[kswang1029]

@pford @veggiesaurus with this doc https://docs.google.com/document/d/1_eo3kmzHrMGDLuYmbMi6RQHytf0NiGyR_igRlBFjssc/edit I tried to find common properties of the casa region format and the ds9 region format. Please have a look. There are properties that are supported in one but not the other. Perhaps what we can do in CARTA is that we only support common properties as listed in the document. Please feel free to share your thoughts. : )

[veggiesaurus]

Since we currently only support region colours, line thicknesses and dash length on the frontend, I'd suggest we start with just those three properties. The colour could be sent as it is defined in the file, as a string, and then interpreted in the frontend (either hex, an RGB string or a standard color). The thickness should be in pixels. The only hitch is that the frontend UI currently supports even dash lengths (e.g. 5 on, 5 off), rather than an arbitrary array of dashes. We could still send those to the frontend, though, and once we have support for arbitrary dash lengths, they'd work.

[veggiesaurus]

@pford can we implement the three important parameters (colour, line width and dash) for 1.4?

[veggiesaurus]

@pford for 1.4 we just need the above three (colour, line width and dash). For the rest, we can wait until 1.5 or later

[pford]

@veggiesaurus SET_REGION has the region parameters as separate fields, and I could just add the style parameters. But IMPORT_REGION_ACK and RESUME_SESSION.ImageProperties contain RegionProperties submessage arrays consisting of region_id and RegionInfo, so I would need to add the style parameters to RegionInfo. What do you think about changing SET_REGION to use RegionInfo as a single field? I think we can keep the SET_REGION.region_id field; RegionProperties was added to send a list of regions.

[veggiesaurus]

@pford are you saying we have something like this?

message SetRegion {
    sfixed32 region_id = 1;
    RegionInfo region_info = 2;
}

(and then updating RegionInfo with the new parameters)

I'd also like to change from using RegionProperties to just using a map of region_id to properties:

message ImageProperties {
    string directory = 1;
    string file = 2;
    string hdu = 3;
    sfixed32 file_id = 4;
    RenderMode render_mode = 5;
    sfixed32 channel = 6;
    sfixed32 stokes = 7;
    map<sfixed32, RegionInfo> regions = 8;
    SetContourParameters contour_settings = 9;
}

To me, that makes more logical sense. I suppose we could apply the same approach to the images field in ResumeSession as well 🤔

[pford]

Yes, just to avoid having to change region parameters in two places. But I can also set the backend struct from the SetRegion parameters as before. I am fine with removing RegionProperties in favor of a map.

[veggiesaurus]

Yes, just to avoid having to change region parameters in two places. But I can also set the backend struct from the SetRegion parameters as before. I am fine with removing RegionProperties in favor of a map.

Ok, let's go with the map approach for both region properties and image properties, and make use of region info in the setregion function as you suggested. If you sort out the protobuf branch and backend branch I'll put together a frontend branch. Shouldn't be much trouble

[pford]

I think we should keep the file_id field in SetRegion, not needed in ImportRegionAck and needs to be in ImageProperties (not RegionInfo) for ResumeSession.

[veggiesaurus]

Ok, that sounds sensible

[pford]

I have pushed protobuf branch pam/region_style and backend branch pam/340_region_style. These changes will break the ICD tests and the scripting client if there is a way to set a region. If my protobuf changes look okay to you, I will put a comment in the interface channel.

I cannot set a region (and therefore cannot export it) in this branch until the frontend is done. I'll try testing region import.

[veggiesaurus]

@pford I've created a frontend branch angus/region_icd_changes. I can create and edit regions, and can export them without issue. Resume seems to work fine. However, when I import regions, it looks like it's always receiving an empty region list. The protobuf object's regions field is just {}

I also managed to crash the backend somehow, when deleting regions. But can't reproduce this issue reliably. I'll file an issue for it if I manage to do it again

edit: I wonder if this has something to do with the map key being an int instead of a string 🤔 Probably not, because it seems to work fine for the other maps we use

edit2: The binary message being sent back (ImportRegionAck) is only 10 bytes long, The first 8 of those bytes is the event header, so I think somehow the backend is just sending success: true and nothing else.

edit3: Ok, I see the problem. The map wasn't getting set. I've pushed a commit to your branch that fixes this issue 😄

[pford]

Hmm I made the RegionInfo line_width field an int, but it could be a float in the frontend. The casaviewer and the SAOImageDS9 viewer only allow integral line widths in their GUIs (drop-down or spinner). The imageanalysis code returns line width from a CRTF import as an unsigned int. What should we do?

[veggiesaurus]

Probably fine to keep it an int then. I don't mind making the line width integral in the frontend too

[pford]

Apparently CRTF and DS9 only accept color names and RGB as color strings. Should I convert the hex to rgb, or just send the rgb from the frontend?

edit: I wrote a converter function
edit again: I just needed to strip off the leading '#'!

[pford]

The backend branch is ready to test when the frontend supports it. The message contents in the console look correct.

• The frontend does not send SET_REGION when style parameters are changed; you have to move the region to force this but then all fields are included and correct.
• Only the name is picked up from IMPORT_REGION_ACK, other style params are ignored.
• RESUME_SESSION includes all fields correctly.

[veggiesaurus]

Apparently CRTF and DS9 only accept color names and RGB as color strings. Should I convert the hex to rgb, or just send the rgb from the frontend?

edit: I wrote a converter function
edit again: I just needed to strip off the leading '#'!

Sorry, I completely missed this. The front-end can deal with hex, RGB or colour names. I can easily convert to RGB if you'd prefer? Am already using a battle tested front-end library that handles it

[pford]

All three formats work in the backend as well. For CRTF export, I just had to strip the leading '#' and convert to lowercase, then do the reverse for import. The DS9 code just passes the hex strings frontend->file and file->frontend. The casaviewer and DS9 viewer can import CARTA-produced region files with the hex codes.

[veggiesaurus]

Ok, the frontend is now using the imported region styling. I noticed that regions without a dash are turning into dashed regions when exported to CRTF. Seem to be fine with DS9 regions though.

I'm wondering what the solution is in terms of sending styling information to the backend. It's not really relevant until the backend wants to export it, and I'm a little worried about sending a SET_REGION every time the line width or colour changes (especially because you can pick colours using a colour picker that updates many times a second). I wonder if we should send just send style parameters when we request a region export?

Edit: perhaps the simplest solution: when sending a region export, why don't we just send the regions explicitly, as a map<sfixed32, RegionInfo>? Then the backend just takes the supplied regions and exports them.

[pford]

The backend has to route the code through the Region class for export conversions (pixel to world, apply region to different image), so resending the type, control points, and rotation with ExportRegion adds complexity as the backend will effectively SetRegion (and check for changes for updating data streams) then export. What if we revert RegionInfo to type/points/rotation and create a separate RegionStyle for name/color/linewidth/dashlist? Then use the two submessages as needed; ImportRegionAck is the only one which requires both.

• SetRegion: RegionInfo
• ExportRegion: map<sfixed32, RegionStyle>
• ImportRegionAck: map<sfixed32, RegionInfo> and map<sfixed32, RegionStyle>
• ResumeSession: map<sfixed32, RegionInfo>

Side note, in angus/region_icd_change I cannot change the pixel control points in the Region widget.

[veggiesaurus]

• SetRegion: RegionInfo
• ExportRegion: map<sfixed32, RegionStyle>
• ImportRegionAck: map<sfixed32, RegionInfo> and map<sfixed32, RegionStyle>
• ResumeSession: map<sfixed32, RegionInfo>

This sounds good

Side note, in angus/region_icd_change I cannot change the pixel control points in the Region widget.

Ok, I'll have a look tomorrow morning

[pford]

I updated the protobuf and backend branches with RegionStyle (note that region name was moved from RegionInfo to RegionStyle).

[veggiesaurus]

I updated the protobuf and backend branches with RegionStyle (note that region name was moved from RegionInfo to RegionStyle).

Commit 7d3432cb of angus/region_icd_change should work with your branches now

[pford]

Found and fixed an existing bug, resuming a session with region id 0 sets the cursor not a region. Also fixed importing color by name.

Since CRTF has no dash_list (just SOLID or DASHED), backend sets dashlist={2, 2} on import when linestyle=--. This DASH_LENGTH constant is set in InterfaceConstants.h if it should be another value. So the imported dash_list may not return the original dash_list.

[veggiesaurus]

Found and fixed an existing bug, resuming a session with region id 0 sets the cursor not a region. Also fixed importing color by name.

Since CRTF has no dash_list (just SOLID or DASHED), backend sets dashlist={2, 2} on import when linestyle=--. This DASH_LENGTH constant is set in InterfaceConstants.h if it should be another value. So the imported dash_list may not return the original dash_list.

That's sensible. Do you think we're ready for a PR of frontend and backend?

[pford]

@kswang1029 can annotation regions be matched then exported, like any other region?

[pford]

@kswang1029 never mind about the matched region question, it is in the requirements document.

Another question: In DS9, you can change the coordinate system for a region, e.g. for an FK5 image change a region to FK4 coordinates in the region information. If you export the regions in FK5 coordinates in the export dialog, most regions are exported as fk5 coordinates (global csys). But a ruler or compass will be exported with FK4 coordinates with compass=fk4 or ruler=fk4 even though the global csys is fk5. Should CARTA support this feature for these two regions?
(Note the ruler definition in the DS9 docs does not show the coordinate system but the DS9 SAOImage viewer includes it)

[kswang1029]

@kswang1029 never mind about the matched region question, it is in the requirements document.

Another question: In DS9, you can change the coordinate system for a region, e.g. for an FK5 image change a region to FK4 coordinates in the region information. If you export the regions in FK5 coordinates in the export dialog, most regions are exported as fk5 coordinates (global csys). But a ruler or compass will be exported with FK4 coordinates with compass=fk4 or ruler=fk4 even though the global csys is fk5. Should CARTA support this feature for these two regions? (Note the ruler definition in the DS9 docs does not show the coordinate system but the DS9 SAOImage viewer includes it)

@pford this sounds quite inconsistent and confusing. Not sure the reason why it is implemented that way in ds9. I would suggest we make it consistent in CARTA.