GR32_LowLevel.Mirror PUREPASCAL does not produce expected values

[andersmelander]

The GR32_LowLevel.Mirror* functions are used in various places to clamp values to a given range. Notably, when sampling a bitmap, they are used to avoid out of bounds pixel coordinates.

For example, Mirror(n, 5) where n=[0..15] should return the values 012345432101234.
In other words: Mirror(n, Max) should be a triangle function with a wavelength of Max.

The current implementations has multiple issues:

• The algorithm of the Mirror(Value, Max) PUREPASCAL implementation does not match the algorithm of the assembler implementation. The PUREPASCAL implementation has incorrect handling of the edge conditions, so in the example above, instead of returning
  012345432101234
  it returns
  012345543210012
• The MirrorPow2 implementation does not special case negative values and appears to suffer from the same problem as the Mirror implementation.

[CuriousKit]

How goes the progress in fixing this?

[andersmelander]

I'm busy with other stuff so I'm not working on it (which is why I haven't assigned it to myself)

[CuriousKit]

That's fair. I saw that you made a commit related to the bug and thought initially that the bug had been fixed, but a closer look at the commit messages indicates that it's not all fixed yet. Still, these messages will indicate that the issue is still open.

[andersmelander]

For reference, what we need is a replacement for the current (broken) algorithm:

function MirrorPow2(Value, Max: Integer): Integer; overload;
begin
  if Value and (Max + 1) = 0 then
    Result := Value and Max
  else
    Result := Max - Value and Max;
end;

MirrorPow([0..13], 3)
Expected: 0123210123210
Actual: 0123210301232

[CuriousKit]

I'll take a study of the file myself at some point. I'm curious to know what the function is supposed to do exactly.

[andersmelander]

The Clamp, Wrap, and Mirror functions are primarily used when sampling bitmaps (e.g. resizing, rotating, transforming, etc). They specify how out of bounds pixel coordinates should be handled.
https://graphics32.github.io/Docs/Units/GR32/Types/TWrapMode.htm

[CuriousKit]

Ah I see, thank you. Having played with textures in OpenGL, I can definitely see the importance of getting this function correct then!

[andersmelander]

...and also used by some gradient functions: https://graphics32.github.io/Docs/Units/GR32_ColorGradients/Classes/TCustomGradientPolygonFiller/Properties/WrapMode.htm

[andersmelander]

I can definitely see the importance of getting this function correct then!

I would think so but the bug has been there since the functions introduction in 2007...

[lamdalili]

Sorry I didn't notice that it was resolved in new branch

graphics32/Source/GR32_LowLevel.pas

Lines 1255 to 1265 in a322add

	function Mirror(Value, Max: Integer): Integer;
	var
	Quotient: Integer;
	begin
	if Value < 0 then
	Value := -Value;
	Quotient := DivMod(Value, Max, Result);
	if ((Quotient and 1) = 1) then
	Result := Max - Result;
	end;

Test on GradFills example :
Original mirror :

After correction

[andersmelander]

@lamdalili Thanks but the Mirror function has already been fixed in the LowLevel.Mirror branch:

graphics32/Source/GR32_LowLevel.pas

Lines 1255 to 1265 in a322add

	function Mirror(Value, Max: Integer): Integer;
	var
	Quotient: Integer;
	begin
	if Value < 0 then
	Value := -Value;
	Quotient := DivMod(Value, Max, Result);
	if ((Quotient and 1) = 1) then
	Result := Max - Result;
	end;

What remains to be fixed is the MirrorPow2 function:

graphics32/Source/GR32_LowLevel.pas

Lines 1321 to 1330 in a322add

	function MirrorPow2(Value, Max: Integer): Integer; overload;
	begin
	if Value < 0 then
	Value := -Value;
	if ((Value and (Max + 1)) = 0) then
	Result := (Value ) and Max
	else
	Result := (Max - Value + Max) and Max;
	end;

See: #291 (comment)