-
Notifications
You must be signed in to change notification settings - Fork 309
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
generic device support #319
base: main
Are you sure you want to change the base?
Conversation
Nice one @coolbutuseless! One comment: I tried to test this branch and found I couldn't use Line 220 in 9fe8335
get can't find 'cairopng' .
Move that into the switch maybe? @thomasp85, I'm not sure if you're aware but R graphics look pretty awful out of box on Windows due to lack of anti-aliasing on devices that are supported there. One way around this is to use the devices in the It would be really sweet to be able to set this as my device with |
I made a quick change to @coolbutuseless's branch to show you the difference, rendering with In doing so I noticed one additional flaw this implementation which is you need have the library loaded that contains the device i.e. Edit: |
Can you not already use the Cairo device using the advice here, using
type=“cairo” in the animate call?
https://github.com/ropenscilabs/learngganimate/blob/master/animate.md#type-argument-why-your-animation-is-pixelated-on-windows
…On Sun, Aug 4, 2019 at 9:44 PM Miles McBain ***@***.***> wrote:
I made a quick change to @coolbutuseless
<https://github.com/coolbutuseless>'s branch to show you the difference,
rendering with Cairo::CairoPNG on Windows:
[image: anim_with_anti_aliasing]
<https://user-images.githubusercontent.com/9996346/62439342-d9461a80-b78e-11e9-9e8d-eb4797a0c52c.gif>
In doing so I noticed one additional flaw this implementation which is you
need have the library loaded that contains the device i.e.
get("Cairo::CairoPNG") doesn't work while get("CairoPNG") does.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#319?email_source=notifications&email_token=AEOX2Y7ABFQMH4SOCWLLOYLQC6VY7A5CNFSM4HK2K2Z2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3QVIUY#issuecomment-518083667>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEOX2Y6IZRH2GKNJ7SKPVUDQC6VY7ANCNFSM4HK2K2ZQ>
.
|
@jonspring that's an interesting suggestion I didn't know you could instantiate a Cairo device on Windows using an arg to dev(). I think this PR still stands regardless and the bugs I found still need to be addressed. Also maybe using type this way should make it into the documentation? An example in |
Yeah, the I’m pretty sure I’m not going to accept this PR. Searching for functions passed as strings is really not a good idea IMO. I’ll have to think about a good way to allow arbitrary devices though |
Maybe you're right about When I said I think this PR still stands I meant the concept. The implementation has issues for sure. Could we not just pass a function? If you can create a function that would return the 'current' device for which you use a string now, I think it could work. |
This is a rough implementation of code to allow for more output devices (See Issue #318).
The user already specifies the character string for the 'device'. Rather than the existing 'switch()' statement to pick the device, use 'get()'.
Also allow the user to explicitly specify the file suffix if wanted, but by default choose the natural suffix for the devices that it knows about already.