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

Make RSAthensMorph apply the world renderer canvas scale factor #56

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

Rinzwind
Copy link
Contributor

@Rinzwind Rinzwind commented Feb 4, 2024

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

Rinzwind commented Feb 4, 2024

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:

[ aCanvas
translucentFormSet: (FormSet extent: self extent asIntegerPoint depth: 32 forms: { surface asForm })
at: self bounds origin asIntegerPoint ]

…nvas instead of the world renderer canvas scale factor (which is used as the scale of the canvas in #getCanvas on OSWindowFormRenderer).
… apply […]”): the BitBlt combination rule for drawing the surface on the argument canvas was erroneously changed from 34 (#blendAlphaScaled) to 24 (#blend).
@Rinzwind
Copy link
Contributor Author

Rinzwind commented Feb 4, 2024

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

Here’s a screenshot comparing the first example again:

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

@Rinzwind Rinzwind marked this pull request as ready for review February 4, 2024 16:44
@Ducasse
Copy link
Contributor

Ducasse commented Feb 4, 2024

Super nice :)

@tesonep tesonep merged commit 5beb296 into pharo-graphics:master Feb 19, 2024
2 of 5 checks passed
@tinchodias
Copy link
Contributor

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

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 2e7ff9f and to then see how this pull request requiring Pharo pull request #16090 should be handled.

@tesonep
Copy link
Contributor

tesonep commented Feb 20, 2024

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

jecisc added a commit to jecisc/pharo that referenced this pull request Mar 11, 2024
## What's Changed
* Reimplement some explicitly required methods to speed up shapes by @jecisc in pharo-graphics/Roassal#54
* Make RSAthensMorph apply the world renderer canvas scale factor by @Rinzwind in pharo-graphics/Roassal#56
* Added .gitignore file and Removed DS_Store file by @jordanmontt in pharo-graphics/Roassal#57
* Feature add relative y axis label values by @jordanmontt in pharo-graphics/Roassal#58

## New Contributors
* @jecisc made their first contribution in pharo-graphics/Roassal#54
* @jordanmontt made their first contribution in pharo-graphics/Roassal#57

**Full Changelog**: pharo-graphics/Roassal@v1.03...v1.06
@jecisc
Copy link
Member

jecisc commented Mar 12, 2024

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

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 added a commit to Rinzwind/Roassal that referenced this pull request Mar 13, 2024
…(“Merge pull request pharo-graphics#56 from […]”) to Pharo 12 to maintain compatibility with earlier versions.
Rinzwind added a commit to Rinzwind/Roassal that referenced this pull request Mar 13, 2024
…(“Merge pull request pharo-graphics#56 from […]”) to Pharo 12 to maintain compatibility with earlier versions.
@Rinzwind
Copy link
Contributor Author

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

@jecisc
Copy link
Member

jecisc commented Mar 14, 2024

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

@Rinzwind
Copy link
Contributor Author

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

jecisc added a commit that referenced this pull request Mar 15, 2024
…emversion

Restrict changes done to #drawOn: on RSAthensMorph in pull request #56 to Pharo 12
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