EtheaDev / SVGIconImageList

Three engines to render SVG (Delphi Image32, Skia4Delphi, Direct2D wrapper) and four components to simplify use of SVG images (resize, fixedcolor, grayscale...)
Apache License 2.0
327 stars 96 forks source link

Minor bug in Image32SVGFactory #185

Closed AngusJohnson closed 3 years ago

AngusJohnson commented 3 years ago

Hi again carlo. There's a minor bug in Image32SVGFactory ...

procedure TImage32SVG.LoadFromStream(Stream: TStream);
Var
  OldPos : Int64;
begin
  // read and save the Source
  OldPos := Stream.Position;
  SourceFromStream(Stream);
  // Restore Position
  Stream.Position := OldPos;
  // Now create the SVG
  fSvgReader.LoadFromStream(Stream);  //replaces the line below
  //FImage32.LoadFromStream(Stream); //delete this line
end;
AngusJohnson commented 3 years ago

and here's an updated version of TImage32SVG.PaintTo:

procedure TImage32SVG.PaintTo(DC: HDC; R: TRectF; KeepAspectRatio: Boolean);
begin

  //Define Image32 output size
  FImage32.SetSize(Round(R.Width), Round(R.Height));
  //Draw SVG image to Image32 (scaled to R and with preserved aspect ratio)
  FsvgReader.DrawImage(FImage32, True);

  //apply GrayScale and FixedColor to Image32
  if FGrayScale then
    FImage32.Grayscale
  else if (FFixedColor <> TColors.SysDefault) then
  begin
    if FApplyFixedColorToRootOnly then
    begin
      fSvgReader.RootElement.SetFillColor(Color32(FFixedColor));
      with fSvgReader.RootElement.DrawData do
        if (strokeColor <> clInvalid) and (strokeColor <> clNone32) then
          fSvgReader.RootElement.SetStrokeColor(Color32(FFixedColor));
    end
    else
      FImage32.SetRGB(Color32(FFixedColor));
  end;

  //Opacity applyed to Image32
  if FOpacity <> 1.0 then
    FImage32.ReduceOpacity(Round(FOpacity * 255));

  FImage32.CopyToDc(DC, Round(R.Left), Round(R.Top), True);
end;

Notes:

  1. I removed the call to CenterRect because (given the suppied parameters) all it will do is offset the destination rect.
  2. Image32 always preserves the aspect ratio when drawing the SVG image. (This doesn't apply to SVG images whose widths and heights are explicitly percentages of their containers.) The only way to force a different geometry would be to apply a transformation to the RootElement's DrawData.matrix (which is currently read-only).
carloBarazzetta commented 3 years ago

Ok, but I need to paint an image without maintain aspect-ratio if KeepAspectRatio is false. Using Benchmark if you load a file by "Load Image" button it is rendered inside a TSVGIconImage with streatch True and Proportiona False, that call PaintTo with KeepAspectRatio False. Using TSVG the result is: image Using Image32: image I've tried changing code to obtain a rescaled non-proportional image like this:

procedure TImage32SVG.PaintTo(DC: HDC; R: TRectF; KeepAspectRatio: Boolean);
var
  LSourceRect, LDestRect: TRect;
  LMaxSize: Integer;
begin
  //Define Image32 output size:

  //Define maximum size for better rescaling
  LMaxSize := Max(Round(R.Width), Round(R.Height));
  FImage32.SetSize(LMaxSize, LMaxSize);

  //Draw SVG image to Image32
  FsvgReader.DrawImage(FImage32, True);

  //GrayScale and FixedColor applyed to Image32
  if FGrayScale then
    FImage32.Grayscale
  else if (FFixedColor <> TColors.SysDefault) then
  begin
    if FApplyFixedColorToRootOnly then
    begin
      fSvgReader.RootElement.SetFillColor(Color32(FFixedColor));
      with fSvgReader.RootElement.DrawData do
        if (strokeColor <> clInvalid) and (strokeColor <> clNone32) then
          fSvgReader.RootElement.SetStrokeColor(Color32(FFixedColor));
    end
    else
      FImage32.SetRGB(Color32(FFixedColor));
  end;

  //Opacity applyed to Image32
  if FOpacity <> 1.0 then
    FImage32.ReduceOpacity(Round(FOpacity * 255));

  LSourceRect := TRect.Create(0, 0, FImage32.Width, FImage32.Height);
  LDestRect := TRect.Create(Round(R.Left), Round(R.Top), Round(R.Right), Round(R.Bottom));
  FImage32.CopyToDc(LSourceRect, LDestRect, DC, True);
end;

But the result is not very good... image

AngusJohnson commented 3 years ago

Yes, it looks like you're rendering a proportional image and then stretching the generated raster image to your non-proportional dimensions (using image resampling). And that largely defeats the purpose of SVG images. SVG images that are intended to be stretched non-proportionally have their widths and heights specified in percentages, and Image32 should render these in that fashion. So I don't understand why you'd want to distort images that aren't intended to be rendered in that way. (Nevertheless, I could modify Image32 to do that if I was persuaded that this was particularly useful.)

AngusJohnson commented 3 years ago

I do note that my suggested code above doesn't center the proportionally drawn image, and this fixes that (though of course this doesn't address you preference for image stretching):

procedure TImage32SVG.PaintTo(DC: HDC; R: TRectF; KeepAspectRatio: Boolean);
var
  dx,dy: double;
begin

  //Define Image32 output size
  FImage32.SetSize(Round(R.Width), Round(R.Height));
  //Draw SVG image to Image32 (scaled to R and with preserved aspect ratio)
  FsvgReader.DrawImage(FImage32, True);
  dx := (R.Width - FImage32.Width) *0.5;
  dy := (R.Height - FImage32.Height) *0.5;

  //apply GrayScale and FixedColor to Image32
  if FGrayScale then
    FImage32.Grayscale
  else if (FFixedColor <> TColors.SysDefault) then
  begin
    if FApplyFixedColorToRootOnly then
    begin
      fSvgReader.RootElement.SetFillColor(Color32(FFixedColor));
      with fSvgReader.RootElement.DrawData do
        if (strokeColor <> clInvalid) and (strokeColor <> clNone32) then
          fSvgReader.RootElement.SetStrokeColor(Color32(FFixedColor));
    end
    else
      FImage32.SetRGB(Color32(FFixedColor));
  end;

  //Opacity applyed to Image32
  if FOpacity <> 1.0 then
    FImage32.ReduceOpacity(Round(FOpacity * 255));

  FImage32.CopyToDc(DC, Round(R.Left + dx), Round(R.Top + dy), True);
end;
carloBarazzetta commented 3 years ago

Yes, it looks like you're rendering a proportional image and then stretching the generated raster image to your non-proportional dimensions (using image resampling). And that largely defeats the purpose of SVG images. SVG images that are intended to be stretched non-proportionally have their widths and heights specified in percentages, and Image32 should render these in that fashion. So I don't understand why you'd want to distort images that aren't intended to be rendered in that way. (Nevertheless, I could modify Image32 to do that if I was persuaded that this was particularly useful.)

I agree, and for my opinion is not very useful, but actually the SVGIconImage component can do it when proportional is set to false. So for my opinion in that case we can draw an image with a poor quality, the same i suppose using a normal TImage with proportional... I've committed a new versione of the Factory that can be tested using Benchmark where I added a checkbox to deactivate Proportional to the SVGIconImage.

AngusJohnson commented 3 years ago

Nevertheless, I could modify Image32 to do that if I was persuaded that this was particularly useful. Done (even though I'm yet to be convinced it's useful) 😁.

So far, these changes are only in the repository here: https://sourceforge.net/p/image32/code/ci/master/tree/

And in Image32SVGFactory you'll need to make the following change:

procedure TImage32SVG.PaintTo(DC: HDC; R: TRectF; KeepAspectRatio: Boolean);
var
  dx,dy: double;
begin
  FsvgReader.KeepAspectRatio := KeepAspectRatio; //ADD THIS LINE
carloBarazzetta commented 3 years ago

Perfect, I've aligned all to last version of Image32 and now the rescaling with "distortion" is very good.

carloBarazzetta commented 3 years ago

@AngusJohnson, you have any idea about the problem of the issue #186 ? Thanks!

carloBarazzetta commented 3 years ago

@AngusJohnson another little bug in Image32/SVG: if a color is written like those examples: fill="#90caf9ff" fill="#42a5f5ff" (with two extra ff at the end of the color) is not recognized and return "black").

carloBarazzetta commented 3 years ago

Another problem with this icon, the gray color of the floppy disk are not rendered:

<svg xmlns="http://www.w3.org/2000/svg" width="75" height="75">
 <defs>
  <g id="A">
   <g transform="matrix(0 -1 -1 0 0 0)">
    <rect width="52" height="52" x="-59" y="-58" rx="2.5" opacity=".2"/>
    <rect width="52" height="52" x="-58" y="-58" rx="2.5" fill="#4f4f4f"/>
   </g>
   <rect width="40" height="24" x="12" y="31" rx="2.5" opacity=".2"/>
   <rect fill="#90caf9" width="40" height="24" x="12" y="30" rx="2.5"/>
   <path fill="#42a5f5" d="M12 48v3.5c0 1.384 1.115 2.5 2.5 2.5h35c1.384 0 2.5-1.116 2.5-2.5V48z"/>
   <path d="M20 7v17.5c0 1.386 1.114 2.5 2.5 2.5h27c1.384 0 2.5-1.114 2.5-2.5V7zm20 4h6c1.108 0 2 .892 2 2v8c0 1.108-.892 2-2 2h-6c-1.108 0-2-.892-2-2v-8c0-1.108.892-2 2-2z" opacity=".2"/>
   <path fill="#b7b7b7" d="M20 6v17.5c0 1.386 1.114 2.5 2.5 2.5h27c1.384 0 2.5-1.114 2.5-2.5V6zm20 4h6c1.108 0 2 .892 2 2v8c0 1.108-.892 2-2 2h-6c-1.108 0-2-.892-2-2v-8c0-1.108.892-2 2-2z"/>
   <path fill="#fff" d="M8.5 6C7.115 6 6 7.115 6 8.5v1C6 8.115 7.115 7 8.5 7h47C56.885 7 58 8.115 58 9.5v-1C58 7.115 56.885 6 55.5 6z" opacity=".1"/>
  </g>
 </defs>
 <use href="#A" x="10"/>
 <use href="#A" y="10"/>
</svg>
AngusJohnson commented 3 years ago

Another problem with this icon, the gray color of the floppy disk are not rendered:

Looks fine to me: floppy_disk1 floppy_disk2

  //apply GrayScale and FixedColor to Image32
  if FGrayScale then
  begin
    FImage32.SaveToFile('c:\temp\floppy_disk1.png');
    FImage32.Grayscale;
    FImage32.SaveToFile('c:\temp\floppy_disk2.png');
  end
AngusJohnson commented 3 years ago

@AngusJohnson another little bug in Image32/SVG: if a color is written like those examples:

Thanks, fixed now 😁.

AngusJohnson commented 3 years ago

More change are required to Image32SVGFactory iron out some residual (minor) bugs. This also required changes to Image32's library that I've just uploaded: https://sourceforge.net/p/image32/code/ci/master/tree/source/

procedure TImage32SVG.PaintTo(DC: HDC; R: TRectF; KeepAspectRatio: Boolean);
var
  dx,dy: double;
  color: TColor32;
begin
  //Define Image32 output size
  FImage32.SetSize(Round(R.Width), Round(R.Height));

  //Update FsvgReader BEFORE calling FsvgReader.DrawImage
  if FApplyFixedColorToRootOnly and not FGrayScale and
    (FFixedColor <> TColors.SysDefault) and
    (FFixedColor <> TColors.SysNone) then
      color := Color32(FFixedColor)
  else
    color := clNone32;

  fSvgReader.SetDefaultFillColor(color);
  fSvgReader.SetDefaultStrokeColor(color);

  FsvgReader.KeepAspectRatio := KeepAspectRatio;

  //Draw SVG image to FImage32
  FsvgReader.DrawImage(FImage32, True);

  //assuming KeepAspectRatio = true, prepare to center
  dx := (R.Width - FImage32.Width) *0.5;
  dy := (R.Height - FImage32.Height) *0.5;

  //Apply GrayScale and FixedColor to Image32
  if FGrayScale then
    FImage32.Grayscale
  else if (FFixedColor <> TColors.SysDefault) and
    not FApplyFixedColorToRootOnly then
      FImage32.SetRGB(Color32(FFixedColor));

  //Opacity applyed to Image32
  if FOpacity <> 1.0 then
    FImage32.ReduceOpacity(Round(FOpacity * 255));

  FImage32.CopyToDc(DC, Round(R.Left + dx), Round(R.Top + dy), True);
end;
pyscripter commented 3 years ago

fSvgReader.SetDefaultFillColor(color); fSvgReader.SetDefaultStrokeColor(color);

The default fill color is clBlack and the default stroke color is clNone. Am I missing something?

AngusJohnson commented 3 years ago

Am I missing something?

Yes and no. I should probably rename this function SetOverrideFillColor since it overrides the default Black only when color <> clNone32. Anyhow, it should work properly as it currently is.

AngusJohnson commented 3 years ago

@pyscripter , @carloBarazzetta

  fSvgReader.SetDefaultFillColor(color);
  fSvgReader.SetDefaultStrokeColor(color);

Hi Guys. Just a quick heads-up in case the following changes catch you by surprise. In the latest Image32 update in the repository, I've removed the TSvgReader's SetDefaultFillColor and SetDefaultStrokeColor methods because they're now largely redundant given I've made TElement's DrawData property read/write. (Actually TElement is also renamed TSvgElement to minimize the risk of naming conflicts.) There's a new FindElement method too so it's possible to change specific element properties too.

Anyhow, to implement SetDefaultFillColor with the new code (very quickly off the top of my head, so untested) ...

procedure SetDefaultFillColor(svgr: TSvgReader; color: TColor32);
var
  dd: TDrawData;
begin
    dd := svgr.RootElement.DrawData;
    dd.FillColor := color;
    sgvr.RootElement.DrawData := dd;
end;

var
  svgr: TSvgReader;
begin
  svgr := TSvgReader..create;
  try
    svgr.LoadFromFile(myfilename);
    SetDefaultFillColor(svgr, clYellow32);
    svgr.DrawImage(mybitmap.handle, true);
  finally
    svgr.free;
  end;
carloBarazzetta commented 3 years ago

Hi @AngusJohnson, during porting from version 2 to 3 of Image32 we changed the call to fSvgReader.SetDefaultFillColor(color); to fSvgReader.SetOverrideFillColor(color); so we don't have any calls to SetDefaultFillColor into Image32SVGFactory... I've tested SVGIconImageList with latest Image32 3.1 version and it seems to works fine... I don't understand your suggestions... thank you Carlo

AngusJohnson commented 3 years ago

@pyscripter , @carloBarazzetta

we changed the call to fSvgReader.SetDefaultFillColor(color); to fSvgReader.SetOverrideFillColor(color)

Yeah, that's probably what I meant, any how it's gone too 😜.

AngusJohnson commented 3 years ago

I've tested SVGIconImageList with latest Image32 3.1 version and it seems to works fine...

But did you test it with the latest version in the repository (uploaded yesterday)?

ps: Thanks Carlo for the very encouraging review of Image32 on SourceForge too. Much appreciated.

AngusJohnson commented 3 years ago

Oops, I've just realised, the update is still sitting here waiting to update. I'll do that now.

Edit: Done now, sorry for the confusion.

carloBarazzetta commented 3 years ago

Ok, but yesterday I've released version 3.0.0 of SVGIconImageList aligned to Image32 3.1 version and requested to Embarcadero to publish on GetIt also for upcoming Delphi 11... Now with those changes (ver.3.2) my library don't compile: [dcc32 Error] Image32SVGFactory.pas(218): E2003 Undeclared identifier: 'SetOverrideFillColor' I've tried to apply your suggestions in this way into my Image32SVGFactory.pas unit:

before (Image32 3.1):

  fSvgReader.SetOverrideFillColor(color);
  fSvgReader.SetOverrideStrokeColor(color);

now (Image32 3.2):

   dd := fSvgReader.RootElement.DrawData;
  dd.FillColor := color;
  dd.strokeColor := color;
  fSvgReader.RootElement.DrawData := dd;

But the pyton icon into Demo\svg_examples\Pyton.svg doesn't draw the root element in black: image

I'm doing something wrong? thank you.

carloBarazzetta commented 3 years ago

Can you "restore" SvgReader.SetOverrideFillColor procedure also into Image32 3.2 version so we don't have a compilation problem if a user uses source code of Image32 3.2 from your repo and SVGIconImageList 3.0 ?

AngusJohnson commented 3 years ago

Can you "restore" SvgReader.SetOverrideFillColor procedure also into Image32 3.2

OK. I'll mark it deprecated.

I'm doing something wrong?

I'll check it out and get back to you.

AngusJohnson commented 3 years ago

OK, I've replaced the 2 methods I'd deleted but modified to use the new functionality.

I'm doing something wrong?

I'm not sure why you had problems, it works for me. Try the latest upload (and make sure you use a TColor32 - eg clYellow32 not clYellow).

carloBarazzetta commented 3 years ago

The problem was this: in the previous version of Img32.SVG.Reader.pas there was this code to setup fillcolor and strokecolor:

    //override the image's default color (black)
    if fDefFillColor <> clNone32 then
      di.fillColor := fDefFillColor;
    if (fDefStrokeColor <> clNone32) and
      (di.strokeColor <> clInvalid) then
        di.strokeColor := fDefStrokeColor;

For this reason, I've changed the "deprecated" procedures in this way:

procedure TSvgReader.SetOverrideFillColor(color: TColor32);
var
  dd: TDrawData;
begin
  if not Assigned(RootElement) then Exit;
  if color <> clNone32 then
  begin
    dd := RootElement.DrawData;
    dd.fillColor := color;
    RootElement.DrawData := dd;
  end;
end;

procedure TSvgReader.SetOverrideStrokeColor(color: TColor32);
var
  dd: TDrawData;
begin
  if not Assigned(RootElement) then Exit;
  if (color <> clNone32) and (dd.strokeColor <> clInvalid) then
  begin
    dd := RootElement.DrawData;
    dd.strokeColor := color;
    RootElement.DrawData := dd;
  end;
end;

Now it works fine, also with 3.0 version of SVGIconImageList and Image32 3.2, but you must upgrade your library to change che code of those procedures. (Thanks).

Then I've pushed a new version of my Image32SVGFactory unit that don't uses the deprecated prcedures.

AngusJohnson commented 3 years ago

@carloBarazzetta Thanks, I will update Image32 again, but I've also found a significant bug in the layers unit that has a significant negative impact on performance that I need to address too before I upload again. Cheers (2.30am here so I'm back to bed 💤).

AngusJohnson commented 3 years ago

@carloBarazzetta OK, I think I've finally fixed that bug in the layers unit. https://sourceforge.net/p/image32/code/ci/master/tree/source/

carloBarazzetta commented 3 years ago

Ok, thank you, but I've not found my proposal for deprecated SetOverrideFillColor and SetOverrideStrokeColor as I've proposed 5 days ago... Am I missing something? Those changes are useful only for users that downloads SVGIconImageList ver.3.0 from GetIt but uses your actual version of the library. Thanks.

AngusJohnson commented 3 years ago

Yes, sorry, I forgot to make those minor adjustments. I've queued them up for my next upload. I've spotted anther bug with layers (related to clippaths and hittesting) that I'm in the process of fixing.