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

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

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

Nyan11
Copy link
Contributor

@Nyan11 Nyan11 commented Oct 17, 2024

Fix: #78

@tinchodias
Copy link
Contributor

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
Copy link
Contributor Author

Nyan11 commented Nov 20, 2024

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
Copy link
Contributor Author

Nyan11 commented Nov 20, 2024

Current:
image

  • In red: what the background should be.
  • In pink: where the bottom label should be align

@tinchodias
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@Nyan11
Copy link
Contributor Author

Nyan11 commented Nov 20, 2024

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
Copy link
Contributor Author

Nyan11 commented Nov 20, 2024

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
Copy link
Contributor Author

Nyan11 commented Nov 20, 2024

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:

  • The angle given by #renderAxisIn: (the "angle" variable) for "axis4" equals: 1.5707963267948963
  • The angle given by Float halfPi equals: 1.5707963267948966

@tinchodias
Copy link
Contributor

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
Copy link
Contributor Author

Nyan11 commented Nov 20, 2024

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
Copy link
Contributor

So, I consider this is ready to be merged.

@tinchodias tinchodias merged commit 1cc1eee into pharo-graphics:master Nov 22, 2024
5 checks passed
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.

RSKiviat usePolygon does not work with even axis
2 participants