search

drawpile / Drawpile

A collaborative drawing program
http://drawpile.net/
GNU General Public License v3.0
1.01k stars 129 forks source link

WIP: Add pixel rulers #1302

Closed fluttershydev closed 2 months ago

fluttershydev commented 2 months ago

This adds pixel rulers to the top and left sides of the canvas. This is only implemented for the new renderer, as the previous renderer is going away eventually.

This handles auto scaling the division spacing and font size, and uses the current theme's colors for drawing the ruler.

Current issues:

fluttershydev commented 2 months ago

What it looks like on Windows 11 image

I also profiled it on a modern Intel i9 and 97% of the time is in the QPainter drawing functions, so I didn't do anything to try to avoid recomputing the transforms, as it wouldn't really have any impact. There's 3% taken by getting the font metrics, but the overall time of the paint is so low in a release build that caching that lookup table and invalidating on font change doesn't seem worth the complexity.

fluttershydev commented 2 months ago

Compiler errors are a bit weird. std::array has been constexpr since C++11 and had deduction guides since C++17. std::lround has existed since C++11. The compilers used where that's failing should support that.

fluttershydev commented 2 months ago

Looks good to me! The update thing should probably addressed, but otherwise it's fine to merge. Let me know when you want to do so, it's fine if it's not in a complete state at that point, since it will still let folks test it.

I'll fix the issues you pointed out first.

Kinda wondering if it would make sense to wrap the canvas view into another widget and put the rulers on that instead so that they don't intrude into the canvas itself. Currently, HUD elements like the zoom notification or the small screen mode buttons don't account for the rulers taking extra room and get covered by or squashed against them. It would also escape the canvas view dichotomy and let it work for the QGraphicsView-based canvas as well.

I noticed the notification box issue, but I didn't even think about the small screen buttons as I don't use it in that mode. Wrapping it on the outside would probably be easier than having everything else need to be aware of the extra space. Although I'm not sure the best way to structure that between the MainWindow and CanvasWrapper.

askmeaboutlo0m commented 2 months ago

Although I'm not sure the best way to structure that between the MainWindow and CanvasWrapper.

I don't have anything particular in mind for it. If you want, you can leave it be and I can have a go at it when it's merged, otherwise it should be fine to just do whatever works.

fluttershydev commented 2 months ago

I'm fine with landing as is, I won't have time until next weekend to look into rearranging that.

askmeaboutlo0m commented 2 months ago

Alright! Will merge this in then and see about fiddling with the widget arrangement if I get time.

askmeaboutlo0m commented 2 months ago

And thanks for implementing this!