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

Render major grid over main grid with thicker lines #3032

Closed
wants to merge 2 commits into from

Conversation

iarkhanhelsky
Copy link
Contributor

@iarkhanhelsky iarkhanhelsky commented Apr 22, 2021

This patch should be mostly considered as a feature request and proposal. Because it doesn't feel right for me and I don't really know how to improve it. Although we use this for a while and it fits our needs perfectly.

The thing we need is a second grid over the main grid to see major cells. We call them sectors. There is a fine grid setting in Preferences but I'm honestly tried to find its meaning and found nothing except "Snapping to Fine Grid". So my first idea is to use this value to draw a "major" grid over the main grid. There is "main grid" and "fine grid" why not draw them both? (similar to #368?)

I'm almost sure this is a valid topic for discussion on the forum yet I wanted to show how it affects the current codebase. Because at this point things come a little bit dirty for my taste.

MapRenderer has method drawGrid which defined as

virtual void drawGrid(QPainter *painter, const QRectF &rect, QColor gridColor = Qt::black) const = 0;

The only "visual" option here is gridColor which seems fine. But when I add another one like int gridFine it doesn't look well. Because probably now it would be better to add fineGridColor as well. At this point, I started to think maybe it is better to pass drawing options to the *Render constructors but not into methods directly. But this looks like massive structural changes for the minor feature.

So I kept things as simple as I could.

Added gridFine - new argument to drawGrid method in MapRenderer. For drawGrid this value acts in sense of "major ticks". When gridFine set to non-zero value Orthogonal and Isometric renderers will draw thicker lines with this step.

HexagonalRenderer does not support this because it's unclear what exactly we need to render here as the fine grid.

Some screenshots

photo_2021-04-22 13 47 50
photo_2021-04-22 13 47 53

Update

I reread the thread in referenced issue and tried to add 'Major Grid' setting. See the second commit.

Added gridFine - new argument to drawGrid method in MapRenderer. For drawGrid
this value acts in sense of majorTicks. When gridFine set to non-zero value
Orthogonal and Isometric renderers will draw thichker lines with this step.

HexagonalRenderer does not support this because it's unclear what exactly we
need to render here as the fine grid.
@iarkhanhelsky iarkhanhelsky changed the title Render fine grid over main grid with thicker lines Render major grid over main grid with thicker lines Apr 23, 2021
@bjorn bjorn closed this in a1a9e18 Apr 26, 2021
@bjorn
Copy link
Member

bjorn commented Apr 26, 2021

This patch should be mostly considered as a feature request and proposal. Because it doesn't feel right for me and I don't really know how to improve it.

I think the implementation wasn't so bad, it just had a few issues but as a basic option it's fine to just pass an additional parameter. Fully customizable grids can follow later.

I would have pushed some adjustments here and then merged it, but the pull request did not allow me to push additional changes. So I've merged it locally after making the following changes:

  • Use opacity as differentiator between major and regular grid lines, as opposed to the width. Using the width seems to lead to rendering issues and I think it looks a bit inconsistent.
  • Fixed the behavior for orthogonal maps, where the previous implementation depended on where the grid started rendering.
  • Update view immediately when the major grid is changed.
  • Default the major grid to 10, which I think is a nice default and shows off the presence of this feature.
  • Fixed Preferences dialog tab order and initial selected page, adjusted label and added suffix to value.

Thanks a lot for getting this started!

@iarkhanhelsky
Copy link
Contributor Author

Thanks! It's always a pleasure to contribute to this project.

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.

2 participants