Created attachment 21812 [details] [review]
Patch to add support for edition of the alpha channel

It would be nice to see this added. I'm putting it on the 1.3
milestone so that someone looks at the patch. If it turns out to be
difficult to complete, we can move it.

IMO the enum value should be called GIMP_TRANSFER_ALPHA_MASK(). I am
afraid however that this enum value (not starting with GIMP_ADD) will
break the autogenerated libgimp enums. Did you test what happens when
pdbgen is run after this change is made?

Yes, and everything seemed to work fine. The only differences 
observed were some strings (GLib enums?) like white-mask or alpha-
mask renamed to add-white-mask or add-alpha-mask. I tried grep on the 
full source tree and didn't find any place in which they were used.

No problem on the renaming. I probably remembered incorrectly the 
final name we agreed on.

I only attached a partial diff to avoid the boring part, but I can 
attach the full diff if needed. The affected files are:
  app/core/gimplayer.c
  app/core/core-enums.h
which were included and
  app/core/core-enums.c
  app/pdb/layer_cmds.c
  libgimp/gimpenums.h
  plug-ins/pygimp/gimpenums.py
  plug-ins/script-fu/script-fu-constants.c
  tools/pdbgen/enums.pl
which were not included.

No need to attach the full patch, these are all generated files.

I'm about to apply this and add undo.

Applied the (modified) patch & added undo:

2003-11-26  Michael Natterer  <mitch@gimp.org>

	* app/core/core-enums.[ch]: added enum values
	GIMP_ADD_ALPHA_MASK_TRANSFER and GIMP_UNDO_GROUP_LAYER_ADD_MASK.

	* app/core/gimplayer.c (gimp_layer_create_mask): applied patch
	from Pedro Gimeno which implements the new ADD_MASK type and
	added undo. Fixes bug #127930.

	* app/gui/layers-commands.c: push an undo group around layer mask
	creation & adding since the creation may change the layer now.

	* app/pdb/layer_cmds.c
	* libgimp/gimpenums.h
	* plug-ins/pygimp/gimpenums.py
	* plug-ins/script-fu/script-fu-constants.c
	* tools/pdbgen/enums.pl: regenerated.

I would like this patch to be reverted, because although this is a
nice patch (thanks, Pedro!), I think that it addresses the wrong
issues and it brings several new problems to the user interface and to
the way the program works.  To see the most obvious problems as a
consequence of this patch, try the following:
- Load an image with transparent areas (try several formats such as
  GIF or PNG, especially if saved by other applications than the GIMP)
- "Transfer" the alpha channel to the layer mask
- Discard the layer mask
- Yuck!
Other problems can occur if the mask is not discarded but some areas
of the mask that were black are painted white.  Even if the effect is
mostly visible on fully transparent pixels, it can also be annoying
for partially transparent pixels, because a pixel with very low
opacity can give strange colors if it is made fully opaque.  This
means that if the user "transfers" the alpha channel to a mask, paints
a bit too much of the mask in black and then wants to re-paint some
areas in white to correct the errors (because it may be too late for
undo), then he would have to be very careful along the edges and it
may be very difficult to restore any previous anti-aliasing effects,
for example.

There are other problems related to the consistency of the user
interface: in the menu for creating the mask, the new option to
"transfer" the alpha channel is the only one that has a side effect
and this is not obvious to the user.  Putting this option in a
separate group may solve this problem, but in any case I don't like
the fact that it mixes different concepts.

I think that I have a better solution to this problem, as described in
bug #128118.  That's why I would like this patch to be reverted as
soon as possible, before too many people spend their time on it (e.g.,
translators).  I would then suggest to close this enhancement as
WONTFIX.

I disagree, Your proposal seems like a worse solution. Please reclose
this bug report as FIXED.

I disagree too. I really like Pedro's solution. It has the advantage
that it's extremely simple (bith the code, and from the user's point
of view).

Dave.

I still think that my solution is better for the consistency of the
user interface, and because if presents a more natural way of working
with masks and transparency.  Please think about it for a while.

By the way, I was suprised when this patch was applied so quickly:
this is a new feature and it was controversial anyway.  There was a
discussion about the topic of masks and transparency on the mailing
list some time ago and it did not seem like we reached a consensus
when the discussions were replaced by some other topics.  So I think
that when we are in a feature freeze, applying a patch for a
potentially controversial feature should be avoided.  Or there should
at least be a bit more time for discussion, instead of applying the
patch in CVS less than one day after it was submitted.

I don't want any conflicts over this patch, but please take some time
to think about it.  If you are not convinced about what I propose in
bug #128118, we can start a discussion on the mailing list.  But I
would strongly suggest to revert this patch first, in order to avoid
the propagation of the problems that it creates.

About Raphaël Quinet 2003-11-28 10:16 (the one with transfer and
yuck)... well, there are cases in which that is exactly what you
want: Tribes2 used RGBA PNGs, with A as some other info (reflection
effect, IIRC), and people wanted to transfer A to mask, so it could
be edited easily. I prefer that people have to know what they do
than disallowing things. And that is what exactly you do in your
steps: 1. store alpha somewhere else 2. destroy it. It would be the
same than playing with Curves for Alpha channel: destroy alpha. But
maybe that is what he wants. If not, apply back. Simple, just know
what you do (or learn), no blind poke.

Hi Raphael,

Yes, it might be a good idea to discuss your proposal on the list,
because as it stands I think you are the only one who sees it as a
positive change. It's possible that I am misinterpreting what you are
proposing, so maybe a longer discussion would be good.

I don't think this patch should be backed out, though. I really like
this functionality. It was never clear to me what the difference
between an alpha channel and a layer mask are in any case, and this
seems like a very natural way to handle it. For a bit of background,
this has not come out of nowhere either - originally a script-fu was
written to do this, and pedro turned that into a patch in C for the core. 

I don't recall the thread in question that you are referring to - do
you have a link to the archives?

Please re-mark this bug as FIXED. If we replace this functionality
with your proposal later, it will be a replacement. Ithink we should
discuss your proposal on the list, because as it is, I really don't
understand how it would improve things.

Cheers,
Dave.

The patch wasn't applied quickly. The change has been discussed in all
details on #gimp for a couple of months. We were only waiting for the
implementation and of course when it arrived, it was applied immidiately.

Unfortunately, I have to leave now (meetings at work, then go home) so I
won't be able to continue commenting on this until late this evening or
tomorrow.  But please take some time to think about this and about the
consequences of removing the alpha channel from the image, especially
when painting in the mask in some areas that were previously partially
transparent (I tried - this is extremely frustrating).  I still hope
that after experimenting a bit, you will be convinced that the best
solution is to revert this patch.

I am sorry, I cannot follow you here. We added this option to allow
the user to do just that. If you don't think it is useful, don't do it
then.

There is nothing wrong with the new functionality, especially since it
doesn't change any existing behaviour. It is not even a new way of
creating a layer mask since you could have done the very same thing
before (it was only more tedious). The patch gives the user a
convenient way of doing something that has always been possible. There
is certainly no good reason to revert it.

Just one more quick note before rushing to my meeting:

Dave, I don't have any links to the mail archives right now, but there
was one discussion in August at gimpcon and another one at the
beginning of September (messages from the 9th and 10th on the
gimp-developer list).  One of the last messages in that thread was
from Sven quoting Joao S. O. Bueno saying:
> Instead of changing the alpha channel when adding a layer mask, create 
> an option besides "apply layer mask". I think "Replace layer alpha 
> with mask" would be descriptive enough and add the needed 
> functionality,
This was unfortunately not discussed, and this is what got me started
with the idea that "transfering" the alpha channel was wrong, but
having a different way to apply it would be the best solution.  Then I
thought about a simple comparison (instead of the replacement
suggested by Joao and repeated by Sven today in bug #128118) and I
started working on the patch.

Sven, as explained in bug #128118, any existing ways to remove the
alpha channel are wrong and should be discouraged.  That's why I
insist on having this patch reverted until we have a better solution.
Also, as I also explained in the other bug, it does not really solve
the problem, especially if you try to paint the mask alternatively
with black and white to fine-tune some areas.

I think that we should involve some other users in the discussion,
especially if they are given a chance to compare both ways of
working.  I will try to submit my patch in bug #128118 as soon as
possible.

Ok, I really have to go now...  Sorry.

I don't think that an option to "Replace alpha channel with Mask" is
in any way related to this bug report. This bug report was about
adding a way to transfer the alpha channel to a mask. This is a useful
shortcut for a perfectly good action and it has been implemented. This
bug is thus fixes.