Skip to content
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

DO NOT MERGE: Draft for PaintSweepGradient update #362

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

drott
Copy link
Collaborator

@drott drott commented May 12, 2022

No description provided.

OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@PeterConstable PeterConstable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left various comments.

Per my comment on the image for figure 5.30, I've prepared an alternate HTML:

image

GitHub won't let me attach the HTML file to this comment, so I'll find another way.

@PeterConstable
Copy link
Collaborator

See #363 for suggested alternate image.

@PeterConstable
Copy link
Collaborator

After we've reviewed and have consensus, we still should wait on merging as we'll need to prepare a separate doc for submission to the MPEG AHG as proposed revision for the new edition. (It's too late to make changes in Amd 2.)

@drott
Copy link
Collaborator Author

drott commented Jun 3, 2022

After we've reviewed and have consensus, we still should wait on merging as we'll need to prepare a separate doc for submission to the MPEG AHG as proposed revision for the new edition. (It's too late to make changes in Amd 2.)

Are you suggesting to wait with merging until the MPEG AHG has accepted the proposed change, too?

@drott
Copy link
Collaborator Author

drott commented Jun 3, 2022

See #363 for suggested alternate image.

See comment above, updated figures with more clearly separating the color stops.

@drott
Copy link
Collaborator Author

drott commented Jun 3, 2022

@justvanrossum Would you have time to review this as well and see if this resolves any ambiguities from our discussion in BlackFoundryCom/black-renderer#17? Thanks in advance.

@PeterConstable
Copy link
Collaborator

Are you suggesting to wait with merging until the MPEG AHG has accepted the proposed change, too?

No, that's not what I meant. These revisions are in OFF_AMD2_WD, which was proposed content for Amendment 2 of OFF. But it's too late to make further changes to amendment 2. So, we'll need to prepare a separate document for that.

OFF_AMD2_WD.md Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated
below 0° and equal to or above 360° are not sampled for drawing the rays.

If the ColorLine's extend mode is reflect or repeat and start and end angle are
equal, nothing must be drawn.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep consistent with ISO speak as elsewhere in this doc:

"... nothing shall be drawn."

OFF_AMD2_WD.md Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Show resolved Hide resolved
Copy link
Collaborator

@PeterConstable PeterConstable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some items might need to be reordered. Fig 5.31 and Fig 5.32 are back to back, but 5.32 pertains to the text before 5.31. Fig 5.31 should follow Fig 5.30, but the text that pertains to 5.31 comes earlier.

I've added comments at relevant points below with suggestions.

OFF_AMD2_WD.md Outdated
is aligned with the start angle, and stop offset 1 is aligned with the end
angle, independent of which angle is larger.

NOTE: If the start angle is less than or equal to the end angle, the color line
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note should move after fig 5.31.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of figure 5.30, and if I am not mistaken, in other cases as well, we describe textually what happens, then we illustrate with a figure after that, don't we? In other words, shouldn't we have the note before the figure 5.31 (which is the one for inversion) here as well?

I moved the note, so if I am not mistaken, no we have the following order:

  • Figure 5.30 illustrates...
  • Figure 5.30,
  • NOTE: If the start angle is less than or equal to the end angle...see figure 5.31.
  • Figure 5.31
  • "Not more than one full...see figure 5.32 for an example"
  • Figure 5.32.

OFF_AMD2_WD.md Outdated
230°, using a `ColorLine` with color stops 0 and 1 for yellow and red and pad
extend mode.**

Not more than one full rotation is drawn and there is no overlap in drawing for
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the note earlier about stop angle > end angle here. (Perhaps drop the "NOTE:" prefix?) Then move Fig 5.32 to directly follow (and re-number to 5.31).

The this paragraph ("Not more than..." follows that figure, with fig 5.31 (re-numbered to 5.32) after that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, note moved, but I don't understand the request for renumbering then.

@vlevantovsky
Copy link

Are you suggesting to wait with merging until the MPEG AHG has accepted the proposed change, too?

No, that's not what I meant. These revisions are in OFF_AMD2_WD, which was proposed content for Amendment 2 of OFF. But it's too late to make further changes to amendment 2. So, we'll need to prepare a separate document for that.

These changes can only go into the new Working Draft of the next edition OFF, and we do need to submit an input contribution summarizing the proposed changes.

Copy link
Collaborator

@PeterConstable PeterConstable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on incorporating revisions into OT 1.9.1. Along the way I'm spotting some editorial details.

Partway done; will finish tomorrow.

For sampling and interpolating colors, the color line is aligned to an
infinitely spiraling circular arc around the center point, with arbitrary
radius. The position 0° on the arc means the direction of the positive x-axis,
360° means one full counter-clockwise rotation. The ColorLine's stop offset 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... and 360° means one full counter-clockwise rotation. The color line’s stop offset 0..."

infinitely spiraling circular arc around the center point, with arbitrary
radius. The position 0° on the arc means the direction of the positive x-axis,
360° means one full counter-clockwise rotation. The ColorLine's stop offset 0
is aligned with the start angle, and stop offset 1 is aligned with the end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... is aligned with the start angle and stop offset 1 is aligned..."

(delete the comma)

is aligned with the start angle, and stop offset 1 is aligned with the end
angle, independent of which angle is larger.

Outside the defined interval of the ColorLine, the color value of a position on
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph is repeating information provided in the color line section. We don't repeat this explanation when linear or radial gradients are described; we simply say that at some point the color of the color line is used. I think this paragraph can be deleted.

(In the existing spec, some similar details are explained, but that was motivated by the different semantics for sweep gradients that was being used.)

Lines for more details. In effect, this means that the spiraling circular arc
can be sampled for colors outside the defined ColorLine interval.

To draw the sweep gradient, for each position along the circular arc, starting
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"To draw the sweep gradient, for each position along the circular arc starting from 0° and up to but not including 360°, a ray from the center outward is painted with the color of the color line where the ray intersects the circular arc at that particular angle. Angle positions below 0° and equal to or above 360° are not sampled for drawing the rays."

@@ -577,7 +577,7 @@ A sweep gradient provides a gradation of colors that sweep around a center
point. For a given color on a color line, that color projects as a ray from the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 306 and 307 (note about repeat/mirror modes being special for sweep) need to be deleted.

of 230°.](images/colr_conic_gradient_start_stop_angles.png)

**Figure 5.30 A sweep gradient with start angle of 110° and an end angle of
230°, using a `ColorLine` with color stops 0 and 1 for yellow and red and pad
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... using a color line..."

OFF_AMD2_WD.md Outdated
@@ -1873,6 +1899,10 @@ mapping data; see 5.7.11.4 for details.
Angles are expressed in counter-clockwise degrees from the direction of the
positive x-axis in the design grid.

NOTE: A bias of 1.0 is used in the representation of start and end angles of
sweep gradients. This is not done, however, for angles in rotate and skew
transforms (5.7.11.2.5.11 and 5.7.11.2.5.12).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be nice to explain why this +1.0 bias is only applied for sweep gradient angles and not for other rotate or skew angles

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

NOTE: To allow for representation of +360°, a bias of 1.0 is used in the representation of start and end angles of sweep gradients. ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To allow for representation of +360°, ...

👍

An example would also be nice

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm.

A normal (i.e. non biased) F2Dot14-encoded-fraction-of-half-circle angle (as used in PaintRotate, PaintSkewGradient, etc.) allows to encode -360 <= v < 360 degrees, thus it can't represent a positive full +360 angle.

Whereas the proposed "biased" F2Dot14 angle used in PaintSweepGradient would allow to encode angles in degrees (-360 + 180) <= v < (360 + 180) => -180 <= v < 540.

E.g. an F2Dot14 value of 0x0 means 0 degrees for the non-biased angle, or 180 degrees for the biased angle.
Or a F2Dot14 value of -16384 (-0x4000) means -180 degrees for the non-biased angle, or 0 degrees for the biased angle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yet another example, 225 degree angle represented using the biased F2Dot14 angle:

>>> v = 4096
>>> bits = 14
>>> angle = (v / (1 << bits) + 1.0) * 180
>>> print(angle)
225.0
>>> v2 = (angle / 180 - 1.0) * (1 << 14)
>>> assert v2 == v

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my draft revision (as porting to OT 1.9.1):

NOTE: To allow for a representation of +360°, a bias of 1.0 is used in the representation of stand and end angles of sweep gradients. For example, an F2DOT14 value of -2.0 (0x8000) represents -180°; an F2DOT14 value of 0.0 (0x0000) represents +180°; an F2DOT14 value of 0.25 (0x1000) represents +225°; an F2DOT14 value of 1.0 (0x4000) represents +360°. However, a bias is not used for representation of angles in rotate or skew transforms (5.7.11.2.5.11, 5.7.11.2.5.12).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I wonder if it's really worth adding the extra complexity of having two kinds of F2Dot14 angles, a biased one for Sweep, and the non-biased one for Rotate and Skew. Why is +360° special, and why does it only matter for Sweep and not, say, for Rotate? What angle intervals can't be encoded using the regular angle without bias? If one wants to do a full rotation 0..360 one can always write it as -360..0 and achieve the same visual result, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option could be that we change the type of the angles from Fixed2Dot14 to a new Fixed4Dot12 for example, always a 16-bit type but it would have slightly smaller fractional part, which would still allow up to 3 decimal digits when converted to/from float, and it could allow to encode values between -16.0 <= v < 15.9998, which in degrees means -2880.0° <= angle < +2879.95605°

OFF_AMD2_WD.md Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated Show resolved Hide resolved
OFF_AMD2_WD.md Outdated
@@ -1873,6 +1899,10 @@ mapping data; see 5.7.11.4 for details.
Angles are expressed in counter-clockwise degrees from the direction of the
positive x-axis in the design grid.

NOTE: A bias of 1.0 is used in the representation of start and end angles of
sweep gradients. This is not done, however, for angles in rotate and skew
transforms (5.7.11.2.5.11 and 5.7.11.2.5.12).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my draft revision (as porting to OT 1.9.1):

NOTE: To allow for a representation of +360°, a bias of 1.0 is used in the representation of stand and end angles of sweep gradients. For example, an F2DOT14 value of -2.0 (0x8000) represents -180°; an F2DOT14 value of 0.0 (0x0000) represents +180°; an F2DOT14 value of 0.25 (0x1000) represents +225°; an F2DOT14 value of 1.0 (0x4000) represents +360°. However, a bias is not used for representation of angles in rotate or skew transforms (5.7.11.2.5.11, 5.7.11.2.5.12).

@drott
Copy link
Collaborator Author

drott commented Jun 29, 2022

@PeterConstable Note sure why GitHub does not let me comment directly in the thread above, but just wanted to point you to the typo in the edited version:

NOTE: To allow for a representation of +360°, a bias of 1.0 is used in the representation of stand start and end angles of sweep gradients. [...]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants