pharo-graphics / Roassal

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

Make RSAthensMorph apply the world renderer canvas scale factor #56

Closed Rinzwind closed 7 months ago

Rinzwind commented 8 months ago

This pull request extends RSAthensRenderer to support scaling, and uses that in RSAthensMorph to apply the canvas scale factor (which was introduced in Pharo pull request #15647).

The following screenshot compares the morph opened by RSChartExample’s #example03Plot in images without and with these changes (on a ‘Retina display’ with the canvas scale factor in each set to 2):

In the image on the left, the Roassal shapes are drawn at scale 1 and then the surface form is scaled up, while in the image on the right, the Roassal shapes are drawn directly at scale 2, resulting in better detail in the label character glyphs and lines on the chart (click the screenshot to zoom in).

Note that the changes to RSAthensMorph use messages that are only implemented in Pharo 12.

Rinzwind commented 8 months ago

I converted this pull request to a draft: there’s a difference in the colors of the curves on the above screenshot which doesn’t look right. The following message probably needs to be revised:

https://github.com/pharo-graphics/Roassal/blob/ffd34ceaa6a1aeb61023096225df33a9ce3a3386/src/Roassal/RSAthensMorph.class.st#L86-L88

Rinzwind commented 8 months ago

The color difference has been fixed (commit fc67a2578bc4a0b3). That requires Pharo pull request #16090.

Here’s a screenshot comparing the first example again:

And another one (with RSKiviatExample’s #example06Chemistry):

Ducasse commented 8 months ago

Super nice :)

tinchodias commented 7 months ago

@Rinzwind some idea how to avoid the "FormCanvas(Object)>>doesNotUnderstand: #scale" in Pharo < 12 ? maybe the baseline loads some compatibility package, I didn't check it. Or CI shouldn't run on older pharos on master

Rinzwind commented 7 months ago

I’m not sure whether dropping older Pharo versions for the ‘master’ branch is an option. In issue #55 it is suggested to have a separate ‘Pharo12’ branch. In any case, I would suggest to maybe first force-push ‘master’ back to commit 2e7ff9f96444845d and to then see how this pull request requiring Pharo pull request #16090 should be handled.

tesonep commented 7 months ago

I think the Pharo12 / Pharo11 / master branch will solve the problem and it is the path we want to go. @tinchodias

jecisc commented 6 months ago

Hi @Rinzwind,

I have the impression that this change is breaking Roassal in Pharo 8 to 11 since the scales are introduced in P12.

Do you think there is an easy way to deal with this?

Rinzwind commented 6 months ago

It was previously suggested to have a separate ‘Pharo12’ branch. Alternatively I could add SystemVersion checks like in the analogous pull request for Bloc (Bloc pull request #465).

Rinzwind commented 6 months ago

I added the necessary SystemVersion checks in commit 7d52bc138ac13e84. I can open a pull request for that or you can just merge it directly.

jecisc commented 6 months ago

I added a #scale method in Pharo < 12 returning 1 already but the second check would be nice to have indeed

Rinzwind commented 6 months ago

I opened a pull request for it: pull request #61.