Version the brushes as well?

[Jehan]

Someone opened a bug report. Apparently some of your newer brushes from master would crash libmypaint 1.3.x.
Cf. https://bugzilla.gnome.org/show_bug.cgi?id=785087

Could you harden a bit the brush processing and discard when they can't be processed? Then backport this to libmypaint 1.3 which is being used by GIMP please. :-)

I see a "version" field inside various .myb files, is it the version of the brush format? If so, it would be a good idea to make use of this and increment the version number. I can see that modelling.myb and modelling2.myb (the 2 brushes which crashed libmypaint) were recently updated with "viewzoom" feature. I guess a version increment could have been done as well. And libmypaint could just discard brush files with unknown version,

[briend]

#74 should be preventing this crash as it makes the engine tolerant to unknown settings. Do you need a newer libmypaint 1.3.x?

[briend]

Oh... dang. #74 probably only catches invalid settings, not invalid inputs. That's a bummer. @achadwick, how difficult is it to add this extra check? :-) I think those are the only two brushes that I added the new viewzoom setting on to try to preserve the previous behavior. Previously, zooming in or out would make the brush bigger or smaller automatically (and it seemed like that was an expected thing), so I recreated that effect with viewzoom. If you could try removing the view zoom data and verifying that fixes it, that'd be great (obviously it won't get bigger or smaller though when you zoom):
https://github.com/mypaint/mypaint/blob/master/brushes/classic/modelling.myb#L187
just delete the:

, 
                "viewzoom": [
                    [
                        -2.7699999809265137, 
                        2.7699999172716274
                    ], 
                    [
                        4.150000095367432, 
                        -4.15
                    ]
]

[Jehan]

Yes, if there is a fix already, it would be good to backport it to v1.3 and make a release with this at some point. Otherwise people who will have both libmypaint and libmypaint-2 installed will experience crashes all the time.

Maybe there could be a v1.3 branch in the repository (and not a simple tag) so that you can continue to backport bug fixes on 1.3 (while new features only go on top of master of course). That's what we do on GIMP for instance.

[Jehan]

If you could try removing the view zoom data and verifying that fixes it, that'd be great (obviously it won't get bigger or smaller though when you zoom):

Removing "viewzoom" field does "fix" the crash.

[Jehan]

By the way, even if the engine is made more "tolerant" to unknown fields, it could be wise to make use of the "version" field in the brush format, no? If brushes evolve, at some point, it may be better to simply discard brushes rather than just loading them with features amputated. No?

[briend]

You're probably right, but it seems like this and #74 are bugs and not a reason to increment the brush version, right? In other words we should lock the current brush version (<=3?) and probably never increment to 4 unless the entire format changed. Even if we added something totally new, like a texture bitmap or something, we could probably still stay with version 3 and let old libmypaint fall back to rendering dabs.

[Jehan]

Well if the fallbacks for these new features are considered acceptable (i.e. if you think the brushes are still usable even without these features), then I guess it's ok.

[briend]

It's really a case by case for each brush, and the issue has been here forever. There are brushes that are worthless without pressure sensitivity. I've made some that really do need a tilting stylus. Maybe each input/setting needs a checkbox "is_critical" for that particular brush. Seems a bit overkill though. The brushes I'm working on, I'm adding notes to describe the ideal zoom level and if tilt is needed. I think that should be better than nothing and let a user know why the brush looks terrible :) .

[briend]

Actually, there is one other issue that I'm not sure how to solve. With 2.0 not only did new inputs come over, but the way speed is calculated was changed from being relative-to-pixel to relative-to-hand. So a bunch of brushes now won't work "quite right" on 1.3. It's not that they aren't compatible or anything, but the calibration is off. So 1.3 release needs to use brushes from before those viewzoom commits. This stinks :-(

[Jehan]

Well then I guess you should just version the data directory: share/mypaint/ for libmypaint vs share/mypaint-2/ for libmypaint-2.

That's what we do for GIMP and that looks like it's the only way to have 2 versions of the same brushes (since that will be needed for this calibration issue).

Also really that's a definite change in behavior and therefore the brushes from mypaint-2 should not be readable by libmypaint (v1), since they will look bad, so I'd really suggest a brush version increment. Indeed even though they would be in separate directories, we could imagine people manually moving brushes without thinking twice.
The opposite though is not very nice. It would be better if brushes could be backward compatible. Is the speed difference not automatically computable? Unless that's something which absolutely has to be tweaked manually (for the right feeling?), if this can be recomputed at least approximately, it would be good if libmypaint-2 could recompute speed of v3 brushes (optionally with a terminal warning).

[briend]

Well then I guess you should just version the data directory: share/mypaint/ for libmypaint vs share/mypaint-2/ for libmypaint-2.

What about git release tags? Or is it bad to rely on git for this kind of thing?

Also really that's a definite change in behavior and therefore the brushes from mypaint-2 should not be readable by libmypaint (v1), since they will look bad, so I'd really suggest a brush version increment.

I'm not sure if the "version" field even does anything at the moment. But, not all the brushes will look bad (most didn't even use speed settings). Not even all the brushes with speed settings looked bad. But I do see your point, it'd be great if we had this type of versioning checks already set up.

The opposite though is not very nice. It would be better if brushes could be backward compatible. Is the speed difference not automatically computable? Unless that's something which absolutely has to be tweaked manually (for the right feeling?), if this can be recomputed at least approximately, it would be good if libmypaint-2 could recompute speed of v3 brushes (optionally with a terminal warning).

It might be easy to read old brush files and have some if statements that change the speed calculation to totally preserve the old look-- but it would also preserve the "unwanted" behavior. At what point would an old brush get converted to the new version? On save? Never? Here is some background on this change that might help
#66
https://community.mypaint.org/t/zoom-and-view-rotation-corrections-rfc/465

[Jehan]

What about git release tags? Or is it bad to rely on git for this kind of thing?

Not sure what you mean. Installing automatically the data in a directory versionned with the last git tag?
IMO git tags are ok for moving targets (i.e. development versions), but I imagine you want some stability in stable versions, no? My advice is that you should plan as much as possible what change could happen within the brush format and make whatever changes is needed before the first release of libmypaint 2.0. Then you can have a stable format (not something changing every X commits).

The point is simple: you must always imagine it is possible to have libmypaint and libmypaint-2.0 installed on the same machine at the same time. They are to be considered 2 different libraries from now on and they must not interfere with each other.

There is not a single solution regarding data with this in mind. But I believe that both versionning the data folders and the brush format itself is the safest and cleanest way for the future. In any case, versionning a file format is always a good idea even if you want it to be forward compatible (which is obviously the case with MyPaint brushes since you want to allow loading brushes while simply discarding unknown options). It's good practice.

[Jehan]

For the record, and as mentionned at: mypaint/mypaint#538 (comment)
I am going forward with the mypaint-brushes packaging and this contains brush versionning: https://github.com/Jehan/mypaint-brushes/releases
GIMP is going to depend on this new package (probably as of today) "mypaint-brushes-1.0" so that means that we won't break anymore with new brushes.

Note well that I am still waiting for MyPaint project to take control of mypaint-brushes repository. I only maintain and host it for the time being because we need to go forward and want to make a GIMP release very soon. For this, we will need distributions out there to start packaging mypaint-brushes as soon as possible. But this repository really should live next to MyPaint and libmypaint!

[tomaszg7]

Sorry to revive an old issue, but after discussion with @Jehan I stumbled on the inconsistency in documentation of mypaint-brushes packages while compiling Gimp for myself. On one hand, I can see in https://github.com/mypaint/mypaint-brushes/blob/master/README.md

This are the brushes to be used by libmypaint 2.x, current development version which has no releases to the day of writing. If you want brushes for the libmypaint 1.x releases, do not install mypaint-brushes from master, but from "v1.3.x" branch (or last release tagged with a "v1.x" version).

On the other hand libmypaint 1.5.0-beta.0 release claims to be feature compatible with libmypaint 2.x branch (as per release notes). Moreover releases of mypaint-brushes state The v2.0.x brushes (in general) require libmypaint >= 1.5 to work correctly / not crash.

So, the question is can the v2 brushes be used with lib >= 1.5.0 or whether one should use v1 brushes in this case? Are they fully compatible with current libmypaint or just don't crash and "don't work quite right"? Gimp still depends on v1 brushes but some distros (like Gentoo) force it to use v2 brushes.

[jplloyd]

Sorry, I must have missed the notification.

The readme in mypaint-brushes is outdated, but it's still true that versions of libmypaint before 1.5 are not compatible (in the sense of crash-proof) with the 2.x releases of mypaint-brushes. I will update the readme file.

Versions of GIMP that use libmypaint >= 1.5 should have no issue using 2.x brushes, though if the gimp code still implements against the old api, some of the new inputs (e.g. barrel rotation) will not have any effect.

libmypaint v1.5 is backwards compatible in that you can keep calling the old parts of the API with old brushes, call the old parts with new brushes without crashing (but not necessarily get the expected result), and call the new parts with old or new brushes.

The new parts of the API are identified by the struct MyPaintSurface2 and functions beginning with mypaint_surface2.

[tomaszg7]

Thanks for correcting the readme. It is now more clear.

However the part "not necessarily get the expected result" is a bit troubling from user's perspective. I understand that some new brushes might depend on new features. Are there also some regressions expected, i.e. brushes in v2 pack which would work worse or misbehave compared to their versions in v1 pack? Bottomline - would you say it is recommended to install v2 pack or v1 pack with legacy apps like Gimp?

[jplloyd]

The brush stroke functions in the new API take a couple of additional parameters, including canvas zoom and canvas rotation, with the zoom being used to scale brush speed input values. For this reason (I assume) brushes with existing mappings on speed inputs have been adjusted, and so would not behave exactly the same as their 1.x counterparts when using the old API.

I don't know how noticeable the difference would be, but there would be a difference.

In general, if implementing against the old API, it would be preferable to use the v1 pack, but using the v2 pack should be fine.

[Jehan]

Thanks @jplloyd. On our side, I am noting that we should update to the new API. Any pointer which specific API you are talking about?

[jplloyd]

Thanks @jplloyd. On our side, I am noting that we should update to the new API. Any pointer which specific API you are talking about?

Certainly. At the top level, a few changes are required:

1. Implementing the MyPaintSurface2 interface instead of MyPaintSurface (see mypaint-surface.h)
2. Calling mypaint_brush_stroke_to_2 instead of mypaint_brush_stroke_to (see mypaint-brush.h)
3. Instantiating brushes with mypaint_brush_new_with_buckets(64) instead of mypaint_brush_new (see mypaint-brush.h)

---

MyPaintSurface2 differences

MyPaintSurface2 extends MyPaintSurface with three new function pointers:

    MyPaintSurfaceDrawDabFunction2 draw_dab_pigment;
    MyPaintSurfaceGetColorFunction2 get_color_pigment;
    MyPaintSurfaceEndAtomicFunction2 end_atomic_multi;

The first of the two are mostly the same as their MyPaintSurface counterparts, with the differences being:

• parameters for the new posterize and paint (pigment/spectral blending) settings in draw_dab_pigment
• parameter for weighted "spectral blending" and ordinary pixel value averaging in get_color_pigment
• being able to return multiple invalidation rectangles from end_atomic_multi (used in the MyPaintTiledSurface2 implementation to handle symmetrical painting better).

Only the first two have to be implemented to support mypaint_brush_stroke_to_2

Having glanced through GIMP's current implementation, I don't think the required changes will be major here. The main question will be how to handle the paint (pigment/spectral) setting.

---

mypaint_brush_stroke_to_2 and mypaint_brush_new_with_buckets

The new stroke_to function takes a MyPaintSurface2 pointer, and adds three new parameters:

• viewzoom - used to adjust speed, so that speed is normalized to screen movement rather than the number of pixels in the image.
• viewrotation - allows brushes to work on a rotated canvas as if you rotated a paper in front of you, i.e rotations operate in screen space. I don't know if GIMP has free canvas rotation, but I recall that it is possible to rotate in increments.
• barrel_rotation - for styluses that support barrel rotation (reporting how it is rotated around its own axis).

mypaint_brush_new_with_buckets allocates a fixed array of smudge states - allowing (in a not-so-ergonomic way) for brushes to record individual smudge states for individual subsets of dabs - for use with brushes that have non-random dab offsets.
If a brush is created with mypaint_brush_new, the Smudge buckets setting will not have any effect on it.

In v1.6.0 mypaint_brush_stroke_to_2_linearsRGB was included. It has an identical signature to mypaint_brush_stroke_to_2, but performs color operations in linear space.

---

MyPaintTiledSurface2 (mypaint-tiled-surface.*) can be seen as a reference implementation, but it also includes a few things that are not required to implement MyPaintSurface2, such as symmetry states

I've probably forgotten a few things, but I think that covers most of the changes.