pharo-graphics / Roassal

The Roassal Visualization Engine
MIT License
16 stars 12 forks source link

Rotate the kiviat polygon and fix the bottom label position #79

Closed Nyan11 closed 3 days ago

Nyan11 commented 1 month ago

Fix: #78

tinchodias commented 5 days ago

Oh, more than a month passed already. I checked at the diff online in the web and it's strangely large. It shows me like if all classes were removed and added. Maybe a newline separator changed? we should check in Iceberg and try it.

@Nyan11 Do you have some script snippet to test what changes?

Nyan11 commented 5 days ago

Oh, more than a month passed already. I checked at the diff online in the web and it's strangely large. It shows me like if all classes were removed and added. Maybe a newline separator changed? we should check in Iceberg and try it.

I use a Windows computer. Maybe a problem with CRLF ?

@Nyan11 Do you have some script snippet to test what changes?

The bugs are explained in #78 . TLDR: When the number of axis on kiviat are even, the background is miss-align and the bottom label is not centered.

Here is an example:

kiviat := RSKiviat new.
kiviat addRow: #(3 5 1 2).
kiviat axisNames: #('axis1' 'axis2' 'axis3' 'axis4').
kiviat usePolygon.
kiviat open
Nyan11 commented 5 days ago

Current: image

tinchodias commented 5 days ago

Thanks. In Iceberg, I see these 2 changes:

Screenshot 2024-11-20 at 12 31 37 Screenshot 2024-11-20 at 12 32 13
tinchodias commented 5 days ago

Your script now outputs:

Screenshot 2024-11-20 at 12 33 43

This looks correct. I'm marking this PR as approved, and prefer to wait some days to see if something else can second the approval and merge it.

tinchodias commented 5 days ago

Well, I don't see the button for Approving the PR.

Nyan11 commented 5 days ago

Actually, the label do not allign with the current PR for kiviat of size 6, 12, 18 and 24 (but does for 30).

4 to: 20 by: 2 do: [ :kiviatSize | | kiviat axis values |
    axis := OrderedCollection new.
    values := OrderedCollection new.

    1 to: kiviatSize do: [ :number | 
        axis add: 'axis', number printString.
        values add: (Random new nextBetween: 1 and: 5)
    ].

    kiviat := RSKiviat new.
    kiviat addRow: values.
    kiviat axisNames: axis.
    kiviat usePolygon.
    kiviat open.
].

image

Nyan11 commented 5 days ago

Seem to be fix. I run the following script and did a visual inspection. All labels seem to be correctly align this time (up to kiviat with 36 axis) (even and odd number of axis).

3 to: 36 by: 1 do: [ :kiviatSize | | kiviat axis values |
    axis := OrderedCollection new.
    values := OrderedCollection new.

    1 to: kiviatSize do: [ :number | 
        axis add: 'axis', number printString.
        values add: (Random new nextBetween: 1 and: 5)
    ].

    kiviat := RSKiviat new.
    kiviat addRow: values.
    kiviat axisNames: axis.
    kiviat usePolygon.
    kiviat open.
].
Nyan11 commented 5 days ago

the modification is here (#roundUpTo: in the dictionary and access):

fixLabelPosition: label angle: angle

    | positions gap roundedUpTo |
    roundedUpTo := 0.0001.
    label position: angle cos @ angle sin * radius.
    gap := self labelGapSize.
    positions := Dictionary newFromPairs: {
                         (0 roundUpTo: roundedUpTo).
                         (0.5 @ 0).
                         (Float halfPi roundUpTo: roundedUpTo).
                         (0 @ 0.5).
                         (Float halfPi negated roundUpTo: roundedUpTo).
                         (0 @ -0.5).
                         (Float pi roundUpTo: roundedUpTo).
                         (-0.5 @ 0).
                         (Float pi + Float halfPi roundUpTo: roundedUpTo).
                         (0 @ 0.5) }.
    positions
        at: (angle roundUpTo: roundedUpTo)
        ifPresent: [ :fix |
        label translateBy: label extent * fix + (gap * fix sign) ]
        ifAbsent: [
            gap := angle cos @ angle sin * gap.
            (angle between: Float halfPi negated and: 0) ifTrue: [
                ^ label translateBy: label baseRectangle bottomLeft negated + gap ].
            (angle between: 0 and: Float halfPi) ifTrue: [
                ^ label translateBy: label baseRectangle topLeft negated + gap ].
            (angle between: Float halfPi and: Float pi) ifTrue: [
                ^ label translateBy: label baseRectangle topRight negated + gap ].
            label translateBy: label baseRectangle bottomRight negated + gap ]

EDIT:

The "angle" is given by the method `RSKiviat >> #renderAxisIn:, and it is calculated with a floating point division followed by an addition.

In the case of a kiviat with 6 branches:

tinchodias commented 5 days ago

Great. I didn't noticed first which misalignment you were talking about. But then I did and I appreciate your fix.

BTW, in the second commit there isn't the same normality in the web diff. Did you change Pharo version or computer?

Nyan11 commented 5 days ago

BTW, in the second commit there isn't the same normality in the web diff. Did you change Pharo version or computer?

I did both on the same computer.

First commit:

Pharo 12.0.0 Build information: Pharo-12.0.0+SNAPSHOT.build.1527.sha.a851dbb926634aead9fba401b789d585e3d78c8d (64 Bit)

Second commit:

Pharo 12.0.0 Build information: Pharo-12.0.0+SNAPSHOT.build.1540.sha.89d0e0e998e70f047c7df5eb97f3dc4065e9ee52 (64 Bit)

tinchodias commented 3 days ago

So, I consider this is ready to be merged.