Brush inputs/settings calibrated for viewzoom and viewrotation [WIP] #731
Conversation
|
@briend could you please update your pull requests so I can review it? |
|
@odysseywestra Thanks for chasing these PRs ☺ @briend Can you collapse the whitespace-only commits into zero or one as well, please? A forced push onto this branch is fine for us. I tend to go a step further and prefix sequences of changes with a Flake8-fixing pass, but that's not a MyPaint QA requirement... yet 😄 |
|
@briend seems like your pull request is failing to build on both Tea-CI and Travis-CI. Could you check logs and see if you can correct the problems they are popping up? |
|
I'm pretty sure they are failing because this patch needs the correlating patch to libmypaint, since I'm sending 2 more pieces of data. I'm not sure how this can be resolved, chicken before the egg type problem? |
|
Probably the best way to fix that would to have the CI tools to build with the Contributors Fork of |
briend
force-pushed
the
view-zoom
branch
3 times, most recently
from
d1f8cb0
to
b4f999e
Compare
There was a problem hiding this comment.
Can you rebase against master again please, getting rid of the hunk that duplicates those 4 methods that got moved? Sorry for messing with your patch in that refactor earlier 😐
We should decide where the angle correction for tilts goes too. I've a tilt-sensitive tablet here, so I can test that bit.
lib/layer/data.py
Outdated
| ) | ||
| self._surface.end_atomic() | ||
| self.autosave_dirty = True | ||
| return split |
There was a problem hiding this comment.
Looks like like thee additions crept into the patch by accident during a rebase. These methods got moved into a new base class in master.
| @@ -359,6 +363,8 @@ def motion_notify_cb(self, tdw, event, fakepressure=None): | |||
| pressure = event.get_axis(Gdk.AxisUse.PRESSURE) | |||
| xtilt = event.get_axis(Gdk.AxisUse.XTILT) | |||
| ytilt = event.get_axis(Gdk.AxisUse.YTILT) | |||
| viewzoom = tdw.scale | |||
| viewrotation = tdw.rotation | |||
There was a problem hiding this comment.
Re the angle correction stuff for tilt at line 431 below (sorry, github doesn't allow me to comment there)
If we are capturing view rotation now and passing it to the brush engine, shouldn't that block be moved to the brush engine's stroke_to? The goal here is that brush authors get an ascension relative to the reference angle on the canvas, not the viewport. That needs to still be the case, but it would be better to move it into the brush engine at the same time we change its interface, given that we're changing what xtilt and ytilt mean.
With that in mind, shouldn't viewrotation be flipped too, if the view is mirrored? Same reason as in the correction calculation below ☺
There was a problem hiding this comment.
I see what you mean; it would be better to let the brush engine take care as much as it can with the data it is given, right? One thought I had is that if we ever want a virtual pen rendered along with dab angle (like some other programs do) we'd have to keep (or bring over more) stuff into the gui, duplicating the calculations?
Also, I wonder if other programs interested in the brush engine already have pre-calculated data that would be need to be converted back to "raw"? I could imagine that being really annoying.
Would it make sense to somehow allow raw data OR calibrated data? So the brush engine would just use either one. I don't know how you could do that, but it'd be nice if you could name the data that you fed to the engine "raw_xtilt" vs "calibrated_xtilt", etc. Then the engine could just have an if statement to handle them differently?
There was a problem hiding this comment.
Yes, you're right with the other programs point. It's subtle and evil to change the meanings of tilt on them. Let's keep the meaning as at present.
The right place to hang this, if any optimization is needed, is Brush::stroke_to() in brush.hpp.
|
@briend The refactor I want to do is likely to break this patch again, so lets get your stuff into MyPaint first! 😀 |
|
Sorry about the duplicate lines, I still get confused by rebasing :-). Do you think it makes sense to bundle this all with the barrel rotation stuff, since it touches all the same bits of code and is very tedious to merge? I could make a new branch with view-rotation, zoom, and barrel-rotation all combined and cleaned up if you think that could help: |
|
Let's try doing it one by one so that the commits can be triaged more easily. Do you think that using a glorified namedtuple "EventData" object in the Python code rather than all those separate values would make it more readable? I could add something like that to master first, and it'd give you a place to hang new inputs... |
|
So we can reference them by names? That would be awesome. What about on the libmypaint side is there any chance to make that name based instead of position? Also I wonder if there is any way to make it more lenient so that it's not fatal if you are missing an input or have an extra input? I don't want to hold you up, feel free to make any changes and I can rework these branches. |
This brings zoom level and view-rotation into libmypaint to allow for calibration of various inputs in accord with the state of the zoom and view rotation. Things like speed, ascension, and many others do not currently behave as expected once you zoom or rotate the canvas
|
I fixed up both patches for this and pen-rotation and made pull requests, but I do need to test it a lot more in case I missed another stroke_to somewhere :-) |
|
The right place to unbundle them from a Python object might be at the C++ wrapper layer, Brush::stroke_to(), before it hits libmypaint. Should be reasonably easy: for named access on a |
|
Thanks for these! I notice that view rotation isn't exposed in the brush editor. Should it be? I'd really appreciate a write-up of how to get a brush to have the same on-screen radius at every zoom level. Could it be done in the libmypaint description strings? |
|
@briend
I hope it's OK to assign these to you. |
|
No trouble at all, I'm the one that broke stuff haha. My afternoon is free I'll be able to concentrate on this today :) |
|
I really wish it was more intuitive. Here's some screenshots that should pretty much explain what's going on. Here we are starting off with a big radius brush at 100% zoom. I've dragged the zoom -4.15 so that it matches perfectly with the maximum zoom level. I guess so you can say this is a 1:1 ratio of shrinking to zooming. If you dragged it to -8 then the brush would shrink even faster than you are zooming (which might be useful). Likewise, less than -4.15 would shrink slower than zoom.
Here I've zoomed in to 150% and drawn another dab. It looks the same! You can see the bigger dab to the left and here's 200% and even 550% ALL the way-- 6400% zoom. You better adjust hardness and pixel feather (w/ zoom or manually) if you expect this range of zoom to work.
and here's zoomed out at 37% There are limitations, if you zoom WAY you might hit the brush size limit (unless you started with a very small brush). This seems acceptable since gigantic brushes aren't very useful. |
This is the GUI counterpart to libmypaint changes to correct brushes for view rotation and zoom:
mypaint/libmypaint#70