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

Version the brushes as well? #101

Open
Jehan opened this Issue Jul 18, 2017 · 13 comments

Comments

Projects
None yet
2 participants
@Jehan
Member

Jehan commented Jul 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@briend

briend Jul 18, 2017

Contributor

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

Contributor

briend commented Jul 18, 2017

#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

This comment has been minimized.

Show comment
Hide comment
@briend

briend Jul 18, 2017

Contributor

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
                    ]
]
Contributor

briend commented Jul 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jehan

Jehan Jul 18, 2017

Member

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.

Member

Jehan commented Jul 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jehan

Jehan Jul 18, 2017

Member

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.

Member

Jehan commented Jul 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jehan

Jehan Jul 18, 2017

Member

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?

Member

Jehan commented Jul 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@briend

briend Jul 18, 2017

Contributor

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.

Contributor

briend commented Jul 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jehan

Jehan Jul 19, 2017

Member

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.

Member

Jehan commented Jul 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@briend

briend Jul 19, 2017

Contributor
Contributor

briend commented Jul 19, 2017

@odysseywestra odysseywestra referenced this issue Jul 19, 2017

Open

Move MyPaint Brushes to New Repository #842

0 of 6 tasks complete
@briend

This comment has been minimized.

Show comment
Hide comment
@briend

briend Jul 19, 2017

Contributor

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 :-(

Contributor

briend commented Jul 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jehan

Jehan Jul 19, 2017

Member

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).

Member

Jehan commented Jul 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@briend

briend Jul 19, 2017

Contributor

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

Contributor

briend commented Jul 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jehan

Jehan Jul 19, 2017

Member

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.

Member

Jehan commented Jul 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jehan

Jehan Jan 1, 2018

Member

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!

Member

Jehan commented Jan 1, 2018

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!

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