Support more graphics backends #14

phronmophobic · 2021-11-01T17:48:19Z

Currently, work in progress.

lread · 2021-11-01T20:39:49Z

Interesting!

phronmophobic · 2021-11-01T20:42:28Z

This patch relies on some WIP improvements to membrane, but it mostly works with swing. There's a few differences between how membrane.skia and membrane.java2d fire events, but they shouldn't be too hard to sort out.

phronmophobic · 2021-11-02T04:50:10Z

@lread

I think I'm pretty close to being able to merge these changes in, but I wanted to get your thoughts on a couple things:

I was planning on adding a command line arg and was thinking --graphics with skia and swing as options.
What do you think the default should be? Generally, the skia backend is "better", but the swing backend doesn't require the extra skialib dependency and swing should theoretically also work on windows. For those reasons, I usually make swing the default backend.
This change might conflict with any font changes you're making. I'm happy to wait until those get pulled in so you don't have deal with conflicts, but if you haven't made many changes, then it might be easier to push them first.

lread · 2021-11-02T12:27:28Z

Cool!

Unless you have some reason you want to promote the use of skia, I think a default of swing is fine. We can touch on the pros and cons in the README.

And, very thoughtful of you to think of my WIP, but please do go ahead!

phronmophobic · 2021-11-17T06:49:35Z

I've added cli args for --toolkit so that either skia or java2d (ie. swing) can be used. Right now, I'm stuck on the fact that java2d doesn't recognize "monospace" as a font, but does recognize "monospaced". I think just having membrane ensure that "monospace" works for any toolkit is probably the right way to go, but I'm going to sleep on it.

I feel like the interface of run-term and screenshot is bending away from programmatic usage towards cli usage. I'll create a separate issue with my thoughts.

lread · 2021-11-20T21:55:55Z

README.md

 - [`--font-size`](#fonts) specify font point size (default: 12)
+- [`--toolkit`](#toolkits) specify the toolkit (default: java2d)
 Image format determined by filename extension, supported extensions are `.png`, `.webp`, `.jpeg` and `.jpg`.
 - `--line-delay` if your play script runs a command like `lein repl`, the underlying program might not be ready to accept input immediately.\


Probably a previous bad edit on my part, but:

Image format determined by filename extension, supported extensions are .png, .webp, .jpeg and .jpg.

Belongs to --out option.

yea, I can fix that.

lread · 2021-11-20T21:57:52Z

README.md

 <!-- generated by script/regen-screenshots.sh -->
 ![play-msgcat-font-example](doc/images/screenshot-msgcat-font.png)

+## Toolkits


Worth mentioning somewhere in here what toolkit supports which OSes?

Currently, both java2d and skia seem to support the same OS's that membrane.term says there is support for:

Membrane.term works on macOS and Linux operating systems.

lread · 2021-11-20T22:07:38Z

src/com/phronemophobic/membrane/term.clj

  ([opts]
   (let [opts (merge default-common-opts opts)
-         {:keys [width height color-scheme font-family font-size]} opts
+         {:keys [width height color-scheme font-family font-size toolkit]} opts


It's not clear from reading the code what a toolkit is and what might be valid values.
Perhaps run-term and screenshot need docstrings.

totally agree, see #28

lread · 2021-11-20T22:15:16Z

src/com/phronemophobic/membrane/term.clj

         (case (int key)
           ;; backspace
-           259 (writec-bytes out [0x7f])
+           (8 259) (writec-bytes out [0x7f])


maybe a comment on why we match on 2 key codes?

lread · 2021-11-20T22:20:22Z

The changes are much smaller than I was expecting! Very cool.
Haven't run this at all yet, (can do sometime tomorrow), but the code looks very nice to me.

lread · 2021-11-22T19:51:06Z

README.md

+com.phronemophobic.membrane/skialib-linux-x86-64 {:mvn/version "0.9.31.0-beta"}
+```
+
+If running from the skia project, you can use the `:skia` alias to add the dependency.


Did you mean to say "if running from the membrane.term project"?

Perhaps dropping an example here would be helpful, something like:

clojure -M:membrane.term:skia run-term --toolkit skia

lread · 2021-11-22T19:56:11Z

I assume this is known and expected, but when I run using the java2d toolkit under jdk11, I see reflective access warnings in the terminal from which I launch membrane.term:

$ clojure -M:membrane.term run-term                                                                                                                                                                                                                                                                                     
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by clojure.lang.InjectedInvoker/0x0000000800229c40 (file:/Users/lee/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar) to method sun.font.StandardGlyphVector.getGlyphMetrics(int)
WARNING: Please consider reporting this to the maintainers of clojure.lang.InjectedInvoker/0x0000000800229c40
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

lread · 2021-11-22T20:05:04Z

Small weirdness: When I request --help when using java2d toolkit, a java app is briefly launched and closes (as you can see in my macOS dock):

phronmophobic · 2021-11-22T20:06:58Z

I assume this is known and expected, but when I run using the java2d toolkit under jdk11, I see reflective access warnings in the terminal from which I launch membrane.term:

I thought that was a clojure issue. For other reasons, I just made an update to membrane.java2d to fix reflection warnings. I wonder if updating to the latest version of membrane fixes it?

com.phronemophobic/membrane {:mvn/version "0.9.31.5-beta"}

phronmophobic · 2021-11-22T20:12:29Z

Regarding the dock icon, if we really want to prevent the dock icon from appearing, we can either disable the dock icon altogether (I think it's useful when actually running the term), see https://github.com/phronmophobic/treemap-clj#fixing-the-mac-osx-dock-icon-appearing. Or we can wait to load the toolkit until after we determine if the cli command is run-term.

lread · 2021-11-22T20:18:01Z

dock icon

Yeah, I think it is generally useful. Just found it mildly odd that it showed up when I only requested help. I only mentioned it because I noticed it. Unless it bothers you, might not be worth an issue.

lread · 2021-11-22T20:21:56Z

Yup, your guess was good! Upgrading to membrane to version 0.9.31.5-beta gets rid of the reflection warnings.

phronmophobic · 2021-11-22T20:23:04Z

The dock thing seems like it’s worth an issue. There’s no harm in documenting it.

lread · 2021-11-22T20:32:55Z

Another very minor one: when I do a run-term under skia, I see "Membrane" as my window title but when I use java2d I see "title" as my window title.

Example from macOS:
skia:

java2d:

Oh, and I guess the title bar background is a different color too.

phronmophobic · 2021-11-22T20:49:08Z

100% compatibility between toolkits is a non goal, but using the same title for the window makes sense. How about “membrane.term”? Using the same background color for the title bar also makes sense, but I’m not actually sure how to that. 🙈

lread · 2021-11-22T20:53:14Z

Yeah membrane.term would be the perfect title, I think!
The color for the title bar could simply be a separate git issue that might or might not be addressed someday?

lread · 2021-11-22T20:57:25Z

I thought I'd give this a whirl under Windows.
I've not had any success yet. Any tips?

Membrane.term uses Pty4J which uses winpty... my brain is not understanding what I need to do. Do I need to be launching from Cygwin/MSYS?

phronmophobic · 2021-11-22T21:00:26Z

Unfortunately, my Windows experience is pretty useless. I did use Pty4J rather than wrapping fork_pty myself in hopes that it would automagically work 🤞 .

lread · 2021-11-22T21:13:53Z

Unfortunately, my Windows experience is pretty useless. I did use Pty4J rather than wrapping fork_pty myself in hopes that it would automagically work 🤞 .

Maybe I just need a tweak or two.
I see that Pty4J includes Windows-specific tests.
And I just successfully did a gradlew test from a cmd shell and they seemed to run successfully.
I'll poke around a bit.

lread · 2021-11-22T21:26:38Z

Doh! Membrane.term runs /bin/bash and that's not right for Windows (what is? not sure yet).
But if I, for test purposes, have membrane.term instead run ls -l, I see the following on Windows 10 for:

clojure -M:membrane.term run-term

lread · 2021-11-22T21:42:11Z

Windows support probably deserves its own issue.

phronmophobic · 2021-11-23T01:38:36Z

whoops, https://github.com/phronmophobic/membrane/blob/master/src/membrane/java2d.clj#L918 🙈 . I guess I'll have to make quick update to membrane to fix the title issue.

phronmophobic · 2021-11-23T01:42:22Z

Ok, I think I addressed all the comments that don't have their own separate issues. Everything look good now 👍 ?

lread · 2021-11-23T21:36:49Z

The font rendering on Linux for java2d is rough compared to skia.

(this could certainly be extracted to a separate issue)

Here are a couple of samples from Linux Mint using a 20pt font size to zoom in a bit.

clojure -M:membrane.term:skia run-term --toolkit skia --font-size 20

clojure -M:membrane.term run-term --toolkit java2d --font-size 20

Java version:

$java --version
openjdk 11.0.13 2021-10-19
OpenJDK Runtime Environment Temurin-11.0.13+8 (build 11.0.13+8)
OpenJDK 64-Bit Server VM Temurin-11.0.13+8 (build 11.0.13+8, mixed mode)

When using the same version of Java on macOS I don't see the rough font rendering on for java2d:

lread · 2021-11-23T22:59:09Z

My personal take? Merge it! I can extract my last finding to an issue if you agree that it is worth an issue.

phronmophobic · 2021-11-24T07:03:04Z

oof. The linux java2d rendering is pretty rough. I'll go ahead and merge for now, but an extracted issue would be great! It looks like anti-aliasing isn't turned on. Maybe it's just a simple config issue 🤞 .

Edit: I just created the issue at #32.

phronmophobic changed the title ~~Support more backends~~ Support more graphics backends Nov 1, 2021

phronmophobic added 5 commits November 16, 2021 20:26

Use backend agnostic functions.

b9e2e30

No platform-agnostic save-image function yet.

ef0350d

Fixes for swing support.

19b93b8

Use clojars version.

caa3bdd

Reify toolkit so that either skia or java2d can be used.

595d790

phronmophobic force-pushed the java2d branch from 218b4ab to 595d790 Compare November 17, 2021 06:43

phronmophobic added 8 commits November 19, 2021 12:35

latest toolkit changes.

9f8da59

Merge branch 'main' into java2d

494bbe6

fix calling run-term with no args.

edfcaed

Removing dead code.

ffe13a5

skialib is no longer a required dependency.

4a88c3f

Adding docs for --toolkit.

f3ba67e

Adding --toolkit description to run-term Readme docs.

f076640

Adding some extra readme headers.

9a6b1d3

lread reviewed Nov 20, 2021

View reviewed changes

lread reviewed Nov 22, 2021

View reviewed changes

This was referenced Nov 22, 2021

When using java2d tookit --help a java app is briefly launched and closes #30

Open

skia and java2d toolkits show different color title bars for run-term #31

Open

phronmophobic added 5 commits November 22, 2021 17:24

Fixed wandering doc string.

fe48374

Added comment on keypress keycodes.

cf6a701

Fixing readme typo and adding example.

b31ea91

bumping membrane version. fixes illegal reflections.

1d99ad6

Set window title to membrane.term.

33a7a69

phronmophobic merged commit bbd1c73 into main Nov 24, 2021

phronmophobic mentioned this pull request Nov 24, 2021

Java2d toolkit looks noticeably worse on linux. #32

Open

Support more graphics backends #14

Support more graphics backends #14

Uh oh!

Conversation

phronmophobic commented Nov 1, 2021

Uh oh!

lread commented Nov 1, 2021

Uh oh!

phronmophobic commented Nov 1, 2021

Uh oh!

phronmophobic commented Nov 2, 2021

Uh oh!

lread commented Nov 2, 2021

Uh oh!

phronmophobic commented Nov 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phronmophobic Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lread commented Nov 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lread commented Nov 22, 2021

Uh oh!

lread commented Nov 22, 2021

Uh oh!

phronmophobic commented Nov 22, 2021

Uh oh!

phronmophobic commented Nov 22, 2021

Uh oh!

lread commented Nov 22, 2021

Uh oh!

lread commented Nov 22, 2021

Uh oh!

phronmophobic commented Nov 22, 2021

Uh oh!

lread commented Nov 22, 2021

Uh oh!

phronmophobic commented Nov 22, 2021

Uh oh!

lread commented Nov 22, 2021

Uh oh!

lread commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phronmophobic commented Nov 22, 2021

Uh oh!

lread commented Nov 22, 2021

Uh oh!

lread commented Nov 22, 2021

Uh oh!

lread commented Nov 22, 2021

Uh oh!

phronmophobic commented Nov 23, 2021

Uh oh!

phronmophobic commented Nov 23, 2021

Uh oh!

lread commented Nov 23, 2021

Uh oh!

lread commented Nov 23, 2021

Uh oh!

phronmophobic commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phronmophobic Nov 20, 2021 •

edited

Loading

lread commented Nov 22, 2021 •

edited

Loading

phronmophobic commented Nov 24, 2021 •

edited

Loading