Skip to content

Add grid overlay feature with configurable cell size and labels#527

Open
PollenGeo wants to merge 13 commits into
ome:masterfrom
PollenGeo:feature/grid-overlay
Open

Add grid overlay feature with configurable cell size and labels#527
PollenGeo wants to merge 13 commits into
ome:masterfrom
PollenGeo:feature/grid-overlay

Conversation

@PollenGeo

Copy link
Copy Markdown

Hi, We welcome your comments and would like to add the following

  • Created GridOverlay class for rendering grid lines and labels
  • Added grid control panel with collapse/expand functionality
  • Configurable cell size (5000-10000px) with slider and number input
  • quadrant labels (A1, B2, etc.)
  • Non-interactive grid layers (allows ROI tools to work)

@will-moore

Copy link
Copy Markdown
Member

This looks very nice. Thanks for the contribution.

It seems to me that 5000 pixels is quite large for the smallest grid. I could imagine that much smaller tiles could be useful, down to maybe 500 or even less? Although it's tricky to allow smaller values without making the slider increment also smaller (and harder to pick a nice round number).

Testing with Image 6416 on merge-ci...

Screenshot 2026-05-07 at 07 17 03

As a first time contributor to OME, could I ask you to fill in the Contributor License Agreement at https://ome-contributing.readthedocs.io/en/latest/cla.html.
Many thanks.

@pwalczysko pwalczysko self-requested a review May 7, 2026 08:50
Comment thread src/viewers/ol3-viewer.js Outdated
if (this.gridOverlay) {
if (!this.gridOverlay.isEnabled()) {
// Show grid with configured cell size
this.gridOverlay.showGrid(5, this.gridCellSize, this.gridShowLabels);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed to specify 5 here for lineWidth since a default is specified in the gridOverlay itself.
I found it hard to track down where the 5 was coming from when I changed the default to 2 in the gridOverlay (which was ignored!).

I actually prefer a lineWidth of 2. Maybe this could be configurable in the UI. e.g. option of 1, 2, or 5 px?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, removed the hardcoded 5 so the default from GridOverlay is now used. Making lineWidth configurable in the UI is a good idea, should I include it in this PR or open a separate one?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include it here would be nice if possible? thanks

Comment thread src/viewers/ol3-viewer.js Outdated
});

// Inicializar grid overlay
setTimeout(() => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that sometimes this timeout is not long enough and that this.viewer is still null after 1500 ms, so the gridOverlay is not created.

A better place to initialise the gridOverlay is at the end of the imageDataReady() function above after this.initViewer(); has been called. Right after this:

            // mdi needs to switch image config when using controls
            this.getContainer().find('.ol-control').on(
                'mousedown',
                () => this.context.selectConfig(this.image_config.id))

@will-moore

Copy link
Copy Markdown
Member

Screenshot of grid size of 500 px with lineWidth of 2:

Screenshot 2026-05-07 at 11 38 28

@Tom-TBT

Tom-TBT commented May 8, 2026

Copy link
Copy Markdown
Contributor

With the first test, I had some display issues:

  • If I modified the grid size, deactivate/re-activate the grid, the red lines aren't showing anymore. It comes back if after changing the grid size.
image
  • activating the grid, then opening the ROI tab, the grid goes away. Deactivating/re-activating the grid makes it come back

About the UI, when a single image is open, the button is discrete enough IMO, but it shows up in an odd place when there's more than one image opened:
image

Comment thread src/viewers/viewer/grid-overlay.js Outdated
Comment thread src/viewers/viewer/grid-overlay.js Outdated
Comment on lines +59 to +70
if (customCellSize) {
cellSize = customCellSize; // ← USE THE VALUE PROVIDED
} else {
// Fallback: auto-calculate only if not provided
if (Math.max(width, height) > 50000) {
cellSize = Math.floor(Math.min(width, height) / 6);
} else if (Math.max(width, height) > 10000) {
cellSize = Math.floor(Math.min(width, height) / 10);
} else {
cellSize = Math.floor(Math.min(width, height) / 12);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recommend the use an auto calculated size for reproducibility.
I also haven't seen it in the plugin, so you should remove these conditions

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, removed the auto-calculation logic. Now cellSize only uses the custom value provided.

Comment thread src/viewers/ol3-viewer.js Outdated
Comment on lines +1821 to +1854
/**
* Update grid line width
* @memberof Ol3Viewer
*/
updateGridLineWidth() {
if (this.gridOverlay && this.gridEnabled) {
this.gridOverlay.updateLineWidth(parseInt(this.gridLineWidth));
}
}

/**
* Update grid cell size
* @memberof Ol3Viewer
*/
updateGridCellSize() {
if (this.gridOverlay && this.gridEnabled) {
// Ensure value is within range
if (this.gridCellSize < 5000) this.gridCellSize = 5000;
if (this.gridCellSize > 10000) this.gridCellSize = 10000;

// Update grid with new size
this.gridOverlay.updateCellSize(parseInt(this.gridCellSize));
}
}

/**
* Toggle grid labels
* @memberof Ol3Viewer
*/
toggleGridLabels() {
if (this.gridOverlay && this.gridEnabled) {
this.gridOverlay.toggleLabels();
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation level is one too much. Fix it please

@will-moore

Copy link
Copy Markdown
Member

Regarding Tom's comment on the "Grid" button. It would be nicer if you could find or create a grid icon, so that the button can be consistent with the others in the viewer "toolbar"?

Minor comment: In the popup panel, I'm not sure you need the "Medium, Large, Very Large" labels? I think the slider is clear enough without them.

@joshmoore

Copy link
Copy Markdown
Member

Longer-term suggestion: store the grid state (on/off & size) as an annotation that is automatically detected.

Comment thread src/viewers/viewer/grid-overlay.js Outdated
this.config.labelSize = labelSize;
this.config.showLabels = showLabels;

console.log('Cell size:', cellSize, 'px');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently logging Cell size: true px here, and then it hangs below in an infinite loop.

Comment thread src/viewers/ol3-viewer.js Outdated
if (this.gridOverlay) {
if (!this.gridOverlay.isEnabled()) {
// Show grid with configured cell size
this.gridOverlay.showGrid(this.gridCellSize, this.gridShowLabels);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First argument for showGrid() should be lineWidth

Comment thread src/viewers/ol3-viewer.html
PollenGeo added 4 commits June 9, 2026 10:39
- Updated the grid control button layout and functionality.
- Removed the fixed size presets (Medium, Large, Very Large).
- Added a new option to customize the grid line thickness.
Updated grid overlay configuration and functionality for improved performance and usability.
Single layer — Two separate VectorLayers (grid + labels) merged into one, using a style function that branches on gridType.
No className — Removed custom class names that were creating a separate canvas intercepting pointer events and blocking ROI drawing.
insertAt() instead of splice() — Replaced direct array manipulation with the proper Collection API, fixing the broken enable/disable toggle.
Updated grid line width to 2 and modified grid display logic to use the line width parameter.
@PollenGeo

Copy link
Copy Markdown
Author

With the first test, I had some display issues:

  • If I modified the grid size, deactivate/re-activate the grid, the red lines aren't showing anymore. It comes back if after changing the grid size.
image * activating the grid, then opening the ROI tab, the grid goes away. Deactivating/re-activating the grid makes it come back

About the UI, when a single image is open, the button is discrete enough IMO, but it shows up in an odd place when there's more than one image opened: image

Hello, thank you very much for all your comments. We look forward to your feedback. Regarding the floating icon in the multi-view mode, we’re not sure whether it should be placed in the Zoom.js toolbar section or if you have any other recommendations. We’re also working on saving the grid’s state (enabled/disabled and size) as an automatically detected annotation.

@will-moore

Copy link
Copy Markdown
Member

Everything looks good.
You still need to add back the whole <div class="${image_config.image_info.dimensions.max_t > element. (T-slider).

Try to add your grid-control-panel under the ol-overlaycontainer-stopevent element and add the ol-unselectable ol-control classes to your grid-control-panel.

@will-moore

Copy link
Copy Markdown
Member

I opened PollenGeo#1 against this branch which should address most of the outstanding issues.
Once that's merged, we can test this PR again, and should get a cleaner idea of the complete code changes. There might still be some tidying up to do (e.g. unused CSS, docstrings etc).

This prevents controls buttons overlapping, but is also nicer to have slightly bigger viewers
Grid overlay thanks Will, these are excellent changes, and we will continue to study them and review the recommendations.
@will-moore

Copy link
Copy Markdown
Member

Now that my changes are merged, it's easier to see the changes and cleanup that I missed.

E.g. need to check for unused css classes. I found these don't seem to be used anywhere now and can be removed. grid-header, grid-size-labels, grid-lw-btn.

Just give it a "final" test and check that everything is working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants