After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 760348 - Child layers in exported OpenRaster (ORA) files have the wrong offsets in MyPaint
Child layers in exported OpenRaster (ORA) files have the wrong offsets in MyP...
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: Plugins
git master
Other All
: Normal normal
: 2.10
Assigned To: GIMP Bugs
GIMP Bugs
Depends on:
Blocks:
 
 
Reported: 2016-01-09 07:44 UTC by Daniel Sabo
Modified: 2016-03-28 22:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Example XCF file being exported (753 bytes, application/x-gzip)
2016-01-09 07:44 UTC, Daniel Sabo
  Details
Exported ora when loaded in Gimp (465 bytes, image/png)
2016-01-09 07:45 UTC, Daniel Sabo
  Details
Exported ora when loaded in MyPaint (git-master 1/8/2016) (2.55 KB, image/png)
2016-01-09 07:46 UTC, Daniel Sabo
  Details
plug-ins: openraster: Ignore the x,y values of layer groups (2.67 KB, patch)
2016-01-10 06:15 UTC, Daniel Sabo
none Details | Review

Description Daniel Sabo 2016-01-09 07:44:30 UTC
Created attachment 318564 [details]
Example XCF file being exported

When exporting layer groups to ORA files both the group and their children get (x,y) values relative to image space (0,0). This results in everything being shifted when loading in MyPaint because it expects children to be relative to the parent's (x,y) values.

Changing this will probably impact the ORA loader too, because Gimp exported ORAs look correct when loaded again in Gimp.

MyPaint files look fine in Gimp because it never sets (x,y) for it's layer groups.
Comment 1 Daniel Sabo 2016-01-09 07:45:31 UTC
Created attachment 318565 [details]
Exported ora when loaded in Gimp
Comment 2 Daniel Sabo 2016-01-09 07:46:25 UTC
Created attachment 318566 [details]
Exported ora when loaded in MyPaint (git-master 1/8/2016)
Comment 3 Michael Schumacher 2016-01-09 12:18:56 UTC
Is there anything about this in the spec?
Comment 4 Daniel Sabo 2016-01-09 12:50:31 UTC
https://wiki.freedesktop.org/www/Specifications/OpenRaster/Draft/LayersStack/#stackelement

It just says that the stack element is allowed to have an x and y value, it doesn't say what the effect of setting them is. I would be inclined to assume "what MyPaint does" is the one true specification but CCing someone in here who could fix the wiki would be good too.
Comment 5 Massimo 2016-01-09 15:58:09 UTC
(In reply to Daniel Sabo from comment #1)
> Created attachment 318565 [details]
> Exported ora when loaded in Gimp

Exporting GroupTest.xcf.gz as GroupTest.ora and
loading it in krita shows the two squares overlapping
though
Comment 7 Daniel Sabo 2016-01-10 06:15:15 UTC
Created attachment 318637 [details] [review]
plug-ins: openraster: Ignore the x,y values of layer groups

So given the conflicting interpretations I think the best compromise value is to not set x,y on <stack> elements. Group offsets don't seem to do anything significant in Gimp and any non-zero value will confuse some existing software.
Comment 8 Andrew Chadwick 2016-01-11 10:41:46 UTC
Hi there. Michael Schumacher asked me to have a look in this thread.

MyPaint doesn't save offsets for groups because all its layers use the same reference coords internally. Groups don't really have an offset. When saving, we transform data layer offsets to those of the data bbox or the user-defined frame, since MyPaint is an infinite canvas monster.

https://github.com/mypaint/mypaint/blob/41c49b0cb892ac496465e706735240a5991aa1f2/lib/layer/core.py#L764
https://github.com/mypaint/mypaint/blob/41c49b0cb892ac496465e706735240a5991aa1f2/lib/layer/data.py#L404
https://github.com/mypaint/mypaint/blob/41c49b0cb892ac496465e706735240a5991aa1f2/lib/layer/data.py#L411
https://github.com/mypaint/mypaint/blob/41c49b0cb892ac496465e706735240a5991aa1f2/lib/layer/group.py#L471

So MyPaint does that, and software that doesn't really have group offsets can do something similar. But the spec allows groups other than the root to have offsets, and I think it always has.

IMO the only correct loading interpretation should be that offsets are summed all the way back up the stack. And that's what MyPaint does:

Layer group ORA load method, note summation and recursion:
https://github.com/mypaint/mypaint/blob/41c49b0cb892ac496465e706735240a5991aa1f2/lib/layer/group.py#L76

Surface-backed layer ORA load method, note summation:
https://github.com/mypaint/mypaint/blob/41c49b0cb892ac496465e706735240a5991aa1f2/lib/layer/data.py#L127

Krita should probably not be ignoring offsets on layer groups when loading just because MyPaint doesn't write them.

I can update the Openraster spec to make this clearer.
Comment 9 Andrew Chadwick 2016-01-11 11:01:30 UTC
CREATE list thread for discussing a clarification to the OpenRaster specification:

http://lists.freedesktop.org/archives/create/2016-January/005162.html
Comment 10 Daniel Sabo 2016-01-11 11:59:52 UTC
Thanks for coming to clarify. I agree that MyPaint's interpretation of the values is correct, but I do have some doubts that groups should ever have x,y values.

In terms of Gimp's output I don't see any benefit to exporting layers with offsets when it will confuse existing software. Possibly Gimp should read layers with offsets, but I don't like having code that no existing software will test. Is there an official repository of sample ORA files to test against?
Comment 11 Michael Schumacher 2016-01-11 12:48:00 UTC
My understanding is that this is underspecified in the current standard - but I do not know how OpenRaster is supposed to evolve, maybe it is expected to be shaped by implementation details of software implementing it?
Comment 12 Andrew Chadwick 2016-01-11 13:03:57 UTC
(In reply to Daniel Sabo from comment #10)
> Thanks for coming to clarify. I agree that MyPaint's interpretation of the
> values is correct, but I do have some doubts that groups should ever have
> x,y values.
> 
> In terms of Gimp's output I don't see any benefit to exporting layers with
> offsets when it will confuse existing software.

Same decision we made for GIMP-compatibility, in fact ☺

> Possibly Gimp should read
> layers with offsets, but I don't like having code that no existing software
> will test. Is there an official repository of sample ORA files to test
> against?

This is a thing that should exist, heck I want one for MyPaint too. But my time is limited right now. These days I'd be inclined to dump specification documents, the old reference implementation that nobody's had time to maintain *coughcough*, and a new repo of official test ORA files onto github.

Define "official" for OpenRaster! ☺
Current workflow is entirely based around a low-traffic mailing list and a wiki that's hard to edit. Perhaps it's time to migrate?
Comment 13 Daniel Sabo 2016-03-07 19:29:17 UTC
Are there any objections to going with this patch for now? From the Create mailing list discussion it seems like the closes thing to consensus is that offsets on groups are undefined behavior.

An ORA saved with this patch would be compatible with any future version of OpenRaster that can open existing MyPaint and Krita files.
Comment 14 Andrew Chadwick 2016-03-08 10:43:47 UTC
I think some systems could plausibly store and use them, so we should thrash out that aspect of the spec at LGM.

MyPaint will continue to treat

  * Group1 (x=100, y=700)
    * Group2 (x=-25, y=0)
      * Layer1 (x=0, y=50)

as identical in appearance to

  * Group1
    * Group2
      * Layer1 (x=75, y=750)

That's a reasonable interpretation in my book ☺

It'll save them out in the latter format though, because that's safest.
Comment 15 Daniel Sabo 2016-03-23 19:05:32 UTC
So zeros at least for now:

commit f10067e560fd5b33104bdb680be1a9a1c7458d65
Author: Daniel Sabo <DanielSabo@gmail.com>
Date:   Sat Jan 9 21:56:06 2016 -0800

    plug-ins: openraster: Ignore the x,y values of layer groups (bug 760348)
    
    Neither MyPaint or Krita sets these values when saving, and when
    loading they have conflicting interpretations such that the only
    universally valid value will be zero.

 plug-ins/pygimp/plug-ins/file-openraster.py | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)