Nyan11 commented 2 years ago

In a playground:

1 with Sparta back-end

sectorGeometry := BlAnnulusSectorGeometry new 
        innerRadius: 0;
        outerRadius: 100;
        startAngle: 180;
        endAngle: 120;
        yourself.           

element := BlElement new
        geometry: sectorGeometry;
        background: Color blue;
        yourself.

space := BlSpace new useSpartaOSWindowSDL2Host.
space addChild: element.
space show.

2 with Alexandrie back-end

sectorGeometry := BlAnnulusSectorGeometry new
        innerRadius: 0;
        outerRadius: 100;
        startAngle: 180;
        endAngle: 120;
        yourself.           

element := BlElement new
        geometry: sectorGeometry;
        background: Color blue;
        yourself.

space := BlSpace new useAlexandrieOSWindowSDL2Host .
space addChild: element.
space show.

tinchodias commented 2 years ago

Hello, this rendering diff was deliberate but not well documented... it is tested in one of BARenderTest example canvas.

Do you consider one of them is the correct? I have an opinion but don't want to bias yours.

thanks for reporting an issue.

Nyan11 commented 2 years ago

Hello,

I prefer the idea of Alexandrie where you color the angle rather than make it disappear.

Nyan11 commented 2 years ago

Also, the direction doesn't feel right. I feel like the angle should be choose in the counter-clock direction.

labordep commented 1 year ago

Hello @tinchodias :) @Nyan11 is working with us, I have the same opinion of him. Regardless of the backend, the user should have the same drawing shape result, specialy for theses basics geometrics functions. Now there is a choice of a angle direction and in my opinion the trigonometric circle is a standard wich is good.

tinchodias commented 1 year ago

Hi @Nyan11 and @labordep

I agree all alternative canvas should share the criteria to render this geometry... as it happens with other geometries (I think it's the only exception). I can do the change.

I add to the discussion what Gregory said in the lse-bloc mailing-list, a couple of months ago:

My preference would be to use radians, counterclockwise, starting at point (1,0) on the unit circle. Radians are good for representing periodicity, and counterclockwise projection would map most efficiently to ordinary formulaic results. On the other hand, orientation on a graphic canvas, where the bottom right quadrant is drawn upon, may lend itself to clockwise. I am not sure.

When I had to implement this geometry for Alexandrie, I observed how it behave in Sparta-Cairo and Sparta-Skia, and to my surprise they were different! It is buggy in Sparta-Cairo (I think when inner radius is not zero). And for Sparta-Skia, I didn't agree in how start and end angles are interpreted.

So, as there was already a difference, I added my own: do it like in the Cairo API (e.g. to draw arcs). The angles are specified clockwise and starting at "three o'clock" for our eyes (I say it in this way, I think you will understand what I mean... I interpret it's what Gregory means with "starting at point (1,0) on the unit circle"). This is consistent with the rest of the API where, for our eyes, the (0,0) origin is at top-left and the (1,1) is at bottom-right respect of origin. In comparison to how we read a pie chart in a book, or how we'd draw angles in a piece of paper, this is vertically flipped... but it's the standard way in computer graphics as far as I know. In this context, I consider consistent that angles in computer grow clockwise, because this is "vertically flipped" as for positions.

Don't know if I am clear, sorry if not.

labordep commented 1 year ago

Hi @tinchodias, This is very clear, no problem for me, there is a logical and this is simple to understand. The most important is to have the same behavior between backend (when it is possible).

Thanks in advance for the patch :)

I'm editing why sometimes we are working with different backend : this is temporary but actually in Pharo we don't have choice sometimes to use another backend (reasons can be problem with library, integration problems, we cannot yet connect "fully" to github (Zinc problems) and get all assets, etc.) and we need to advance. When we are switching to the targeted backend (alexandrie) we need to have the same drawing :) This is temporary but necessary for us for this time :(

tinchodias commented 1 year ago

You will have news very soon! Note about the original snippets: the innerRadius and outerRadius must be in the range [0..1], this is said in the setters (aka "mutators").

tinchodias commented 1 year ago

@Nyan11 @labordep please tell me if you can load the change in the Pull Request #190

Or maybe you prefer that I merge the change and test it on the trunk.

fool-in-the-rain commented 1 year ago

I (Gregory) agree with the comment above about flipping vertically, around the X axis. I.e. the unintuitive approach for the annulus sector's clockwise geometry is strangely consistent with the unintuitive way that graphics take place below the X axis.

I am moving from Skia in Pharo 9 now, to Alexandrie in Pharo 11. Can I expect that the annulus sectors will continue in a clockwise direction, or are you planning on flipping it back to conventional geometry, counterclockwise rotation?

The major win I think would be using Radians. The divisibility with radians is more natural. It's more expressive.

tinchodias commented 1 year ago

@fool-in-the-rain So we agree in using clockwise. I will add a radians API to the geometry.

tinchodias commented 1 year ago

These decisions must be specified at least in the geometry's class comment.

For the record, two references:

The Cairo API specifies: "An angle of 0.0 is in the direction of the positive X axis (in user space). An angle of PI/2.0 radians (90 degrees) is in the direction of the positive Y axis (in user space). Angles increase in the direction from the positive X axis toward the positive Y axis.".

This website explains angles in computer graphics in similar way.

tinchodias commented 1 year ago

I commented about this issue in the blog post: https://pharoweekly.wordpress.com/2022/11/14/bloc-update/

fool-in-the-rain commented 1 year ago

That was excellent source material. Confirmation with clarity.

fool-in-the-rain commented 1 year ago

The only other thing I would say, is it might be more convenient, in terms of how code actually gets written, to have the starting angle, and then the difference, that would make up the new angle. It would clean up the development code, and push the algebra back into the math classes. As a matter of fact, I'm pretty sure I saw that before in some other geometric Pharo programming, in Pharo 7. I can't remember where, but I was following that approach, which seemed cleaner to me. So, for example, I start with some angle, whatever it might be, and I ask for an AnnulusSector that extends (2/7) * Pi from there, and I get a shape. I never really cared where the end was. I think it also captures the radial spirit, going around.

tinchodias commented 1 year ago

Thanks @fool-in-the-rain I like the approach of setting a delta to define the sector. I think I read that approach in some other library but don't remember well which one. Maybe the SVG standard, or Skia. I was preparing a new PR related to this issue, but also (trying to keep the API backwards compatible) add the start and end angle radians mutator methods. I can add a endDeltaAngle: easily...

tinchodias commented 1 year ago

... probably as startAngle:deltaEnd:.

About radians, I added startAngleInRadians: and analog for end... I don't like too much, but should be to keep it backward compatible... opinions?

I also wrote a class comment to reduce current ambiguity of what angles and inner and outer mean. And I'm thinking that in fact I would do this:

Define a BlCircleSectorGeometry separated from BlAnnulusSectorGeometry. The code already has IFs to render differently depending on "innerRadius = 0.0".
Remove outerRadius from BlAnnulusSectorGeometry, as it's defined by the extent (size) of the BlElement. Not sure on this, maybe the utility of the outerRadius is easing creation of kind of stacked annulus like:
The pairs (inner,outer) I used there are (0, 0.5) and (0.5, 1), defining 2 levels. (Generated with: http://ws.stfx.eu/8KBTRHG3O63I)

About the class comment, this is what I'm proposing:

I am a sector of an annulus. In mathematics, an annulus is an area bounded by two concentric circles.

The center of the annulus is the middle of the BlElement. The maximum diameter is given by the element's extent.

My instances have an innerRadius and an outerRadius that are in fact not radius but [0..1] coefficients, that, when multiplied by half element's extent determine the above mentioned concentric circles of the annulus.

There is an exception: An inner radius coefficient of 0.0 means it is in fact the sector of a circle.

My instances have a start and end angle to determine the sector of the annulus. The meaning of the angles are:

An angle of 0.0 is in the direction of the positive X axis.

An angle of 90 degrees is in the direction of the positive Y axis.

Angles increase in the direction from the positive X axis toward the positive Y axis.

fool-in-the-rain commented 1 year ago

Agreed, that retaining the (backward compat)ability of specifying annulus sector via start/endpoint angles will prove helpful, for example, when processing columns of raw data.

fool-in-the-rain commented 1 year ago

For what it's worth, I'm experimenting with Fontlab today, which draws vectors, and I notice that it does something that I think we had with Skia: the origin is at 12 o'clock. In other respects, it follows clockwise = positive.

tinchodias commented 1 year ago

Fontlab looks nice but never used it. As another reference, AFAIR, Inkscape UI handles it as I proposed in the PR. I think that an UI or any other "higher-level layer" on top of Bloc might use another criteria for angles, and translate it the the angles that the low-level API (bloc) needs.

fool-in-the-rain commented 1 year ago

Absolutely agreed that the 'higher-level layer' should have its own independent way of charting space and translate it to whatever the supporting layer (Bloc in this case) requires as an API. That's what I do.

tinchodias commented 1 year ago

What do you think of my comment here? https://github.com/pharo-graphics/Bloc/pull/192#discussion_r1036634951

pharo-graphics / Bloc

BlAnnulusSectorGeometry don't display the same shape on Alexandrie / Sparta #187

1 with Sparta back-end

2 with Alexandrie back-end