Integrating skia #344

tushar5526 · 2022-06-11T11:57:56Z

Set up a basic rendering window using GLFW and skia, which supports the following APIs.

Environment

Structure

Event handlers

Keyboard

keyIsPressed
Key
keyCode (not used in p5py)
keyPressed()
keyReleased()
keyTyped() (needs to be fixed on vispy end)
keyIsDown() (not in p5py as of now, we can achieven this using key_is_pressed, so no need to implement it right now)

Mouse

Set up the rendering APIs for basic primitive shapes and set up the event handlers using glfw.

2D Primitives

tushar5526 · 2022-06-20T21:24:15Z

p5/sketch/Skia2DRenderer/handlers.py

+                glfw.MOUSE_BUTTON_MIDDLE: 3
+                }
+
+class PseudoMouseEvent:


@ziyaointl I am using a PseudoEventClass to mimic the data that was generated while using vispy-based events because the events.py is highly dependent on the event object passed by vispy.

Please share your thoughts on this.

I think this is solution has the pro of modifying minimal code in the current codebase, but I think in terms of keeping abstractions between renderers clean we may want to not include information about vispy inside of the skia renderer.

One solution I propose is to first modify the Event, MouseEvent, and KeyEvent classes in p5py so they don't directly rely on the raw events emitted by vispy, but rather specific information relavant from those events (e.g. event.pos, event.buttons). Then, one could write two helper functions that translate either vispy events or glfw events to p5py events. This seems to be similar to what @mrdaybird wrote in his proposal.

Let me your thoughts! If you also think this is a good idea, you can either create a new PR that refactors the existing event classes and rebase this skia PR on top of it, or you can keep the PseudoMouseEvent for now and create the refactor PR later down the road :)

Apologies for the late response, I was shifting back home for summer break.

Yes, this is a good idea. We should refactor the event classes. I will add a helper function in util to generate a p5py event according to the builtins.current_renderer we are using.

I will keep the PseudoMouseEvent now as it is working fine now and I could quickly move to the renderer implementation.

Also, the Renderer and Events modules are completely independent of each other so I am thinking of refactoring the events module later.

No worries at all! I think putting helper functions in either events.py or the respective renderers may be slightly better than putting them into the global util file, but that's just my opionon. The rest of the plan sounds great. Thank you!

mrdaybird · 2022-06-25T18:08:40Z

Checkout moderngl-window for how they implement event handling:
https://github.com/moderngl/moderngl-window/tree/master/moderngl_window/context

tushar5526 · 2022-06-28T21:37:43Z

Also, taking a look at moderngl-window, it seems like a great choice for us to later move on to. It would provide support for multiple windows(glfw, pyglet, pyqt)

Though it's a bit late for me to redo everything now, but we can explore migrating to it later for implementing other Processing APIs easily.

Current implementation of no_loop(), loop(), with proper frame_count Setting up Sketch in base.py Implemented a basic renderer module for testing purposes.

- Added skia to requirements.txt - Refactor events.py, userspace.py and base.py - Add handlers.py for defining callback handlers of GLFW

- Bug Fix in events.py - Deprecated event.change Mouse Events are integarted using the existing events system

- moved_x - moved_y - handler for on_mouse_motion Bug Fixes: - fixed a small bug in events.py MouseButton

- Integrate mouse_dragged()

- Added a dataclass to store the input state

- Ran black on few files for formatting

… as well

- Each renderer instance will have its own style stack - All the style variables are now present in renderer.styles

- Each renderer has its own matrix stack now - transforms.py uses the functions provided by the renderer to update matrix and style settings - This was needed as skia had predefined function for rotate, transform, scale etc. and we didn't need to implement it manually

tushar5526 · 2022-07-01T15:38:41Z

Hey, @ziyaointl, I need your thoughts on this.

primitives.py defines the APIs like arc, ellipse etc. It pre-processes the input and then passes it on to the renderer for rendering.

So, if we have to draw an arc with RADIUS mode in Processing, primitives will handle adjusting the x, y, w, h values before passing it to the renderer.

The way we will pre-process will depend upon whether we are using skia or vispy, so we would need different pre-processing logic for each of them.

How should I achieve this, I am thinking of using if-else to check which renderer we are using and pre-process accordingly.

PS: The functions inside primitives.py are exported, so we can't do something like "conditional exports" because we get to know about the type of renderer later in the process.

Let me know your thoughts on this approach, or if you have some other design in your mind that would work.

- Arc - Ellipse - Circle - Line - primitves.py holds different logic to render primitives depending upon the current renderer.

- We support all parameters same as p5.js - Border Radius is now possible!

- push_style - pop_style - pop_matrix - push_matrix

tushar5526 · 2022-07-05T19:40:40Z

The 2D renderer looks great now, attaching some videos for comparison.

skia.mp4

vispy.mp4

ziyaointl · 2022-07-06T04:37:06Z

@tushar5526 On preprocessing inputs before passing them into the renderer, I think it make a lot of sense to use if-else for now. After we figure out what different preprocessing needs to be done for each renderer, we can potentially tweak the renderer interface and try to move some of the renderer specific logic to each renderer class. But before we actually implement the preprocessing code for skia, it would be pretty hard to know what API tweaks we should make.

Thanks for posting the video! This is incredible progress! It's amazing that there is no aliasing at all. I look forward to the day that we can set skia as the default renderer.

tushar5526 · 2022-07-06T12:45:21Z

@ziyaointl should I wait for you to review and merge this PR, before continuing with my next milestone?

I was thinking of making a branch from this one, and working parallelly on my next milestone.

ziyaointl · 2022-07-07T00:28:28Z

@tushar5526 Thanks for asking! Feel free to continue with another branch, and we can rebase as needed.

ziyaointl · 2022-07-10T00:18:55Z

p5/visualTests/sanityTests/2DSanityTests/test23_skia.py

+        ellipse([mx, my], radius, radius)
+
+if __name__ == '__main__':
+        run(renderer='skia')


Instead of duplicating the tests for each renderer, maybe we could write a function that parses the renderer selection via a command line argument and use that function in all the tests?

Yes, we should do that. Earlier I considered calling both the backend renderers one by one, something like

if __name__ == '__main__': run(renderer='skia') run()

But, this was breaking in tests with noLoop enabled, because we are running the test for 100 frames and then calling exit().

I think it would be easier your way. I will make the updates to the testing suite.

I have updated the testing suite, and the GHA for sanity tests will fail for the skia renderer now as we have not implemented some APIs in skia in this PR. We could either not run tests for skia.

Yeah, I think not running tests for skia is a reasonable approach for now.

p5/sketch/userspace.py

p5/sketch/util.py

p5/core/color.py

p5/core/primitives.py

p5/core/transforms.py

- Added docs at necessary places - Removed matrix_stack from skia Style class - Moved push_matrix, pop_matrix and style2D class to openglrenderer

ziyaointl · 2022-07-12T02:47:20Z

p5/sketch/Vispy2DRenderer/renderer2d.py

@@ -375,3 +376,52 @@ def arc(self, *args):
    def shape(self, vertices, contours, shape_type, *args):
        """Render a Pshape"""
        self.render_shape(PShape(vertices=vertices, contours=contours, shape_type=shape_type))
+
+    def reset_transforms(self):


Looks like this (along with a few other functions) is duplicated between vispy2d and 3d as well. Could you also move them to the common opengl renderer?

Moved the functions, and declared an abstract render function in openglrenderer for shape

ziyaointl · 2022-07-12T02:49:38Z

@tushar5526 Thanks for such a quick response! I added one more small comment, but otherwise I think this PR is in great shape and should be merged :D

p5/sketch/Vispy2DRenderer/renderer2d.py

tushar5526 marked this pull request as draft June 11, 2022 11:58

tushar5526 mentioned this pull request Jun 17, 2022

event.change incorrect behavior as per documentation. #347

Closed

tushar5526 commented Jun 20, 2022

View reviewed changes

tushar5526 force-pushed the integrating-skia branch from 6f41a60 to 988c135 Compare June 21, 2022 18:10

tushar5526 force-pushed the integrating-skia branch 2 times, most recently from 3b198b0 to 0fe20aa Compare June 29, 2022 17:59

tushar5526 added 16 commits June 30, 2022 00:55

Setup GLFW with SKIA

fd98986

Current implementation of no_loop(), loop(), with proper frame_count Setting up Sketch in base.py Implemented a basic renderer module for testing purposes.

Updated the looping mehcanism

06d9aa5

Remove comments

578ff6f

Start with setting up an event handler for Skia integration using GLFW

6a90b19

- Added skia to requirements.txt - Refactor events.py, userspace.py and base.py - Add handlers.py for defining callback handlers of GLFW

update requirements.txt

841ef7f

events.py

8b65c13

- Bug Fix in events.py - Deprecated event.change Mouse Events are integarted using the existing events system

Features added:

c9c2888

- moved_x - moved_y - handler for on_mouse_motion Bug Fixes: - fixed a small bug in events.py MouseButton

handlers.py

8e4e197

- Integrate mouse_dragged()

Implemented double_click functionality

09a06cd

- Added a dataclass to store the input state

Implemented mouse_wheel

c52a90a

- Ran black on few files for formatting

Added set_frame_rate and focused for skia

b3f8df5

Added is_looping

c52336e

Implemented keyEvent handling and added is_looping and set_frame_rate…

ae76474

… as well

Remove debugging statements

6bd18f2

Refactored code as follows

3b0e31d

- Each renderer instance will have its own style stack - All the style variables are now present in renderer.styles

tushar5526 force-pushed the integrating-skia branch from 0fe20aa to 826f9e4 Compare June 29, 2022 19:29

tushar5526 added 2 commits June 30, 2022 01:07

Import p5 in renderer3d

dc00c03

Remove linter errors

37e8d5e

tushar5526 added 2 commits July 4, 2022 17:54

Started basic setup of renderer and primitive shapes

b8029ab

- Arc - Ellipse - Circle - Line - primitves.py holds different logic to render primitives depending upon the current renderer.

Implemented Point API

d66aeea

tushar5526 added 5 commits July 4, 2022 20:03

Enhanced rect AP

9ecc9d2

- We support all parameters same as p5.js - Border Radius is now possible!

Added rect API

c985ee0

Implemented Triangle API

8aa477d

Implemented

a291872

- push_style - pop_style - pop_matrix - push_matrix

Deleted test.py

1c7ec1b

tushar5526 marked this pull request as ready for review July 5, 2022 19:34

tushar5526 added 2 commits July 6, 2022 16:57

Use the new style class everywhere

535be68

Added basic sanity tests for skia

e8ae3ad

tushar5526 mentioned this pull request Jul 7, 2022

Enhancing Skia further #353

Closed

27 tasks

ziyaointl reviewed Jul 11, 2022

View reviewed changes

tushar5526 added 5 commits July 11, 2022 15:01

- Refactored the code to remove unused arguments and comments

7f19ee4

- Added docs at necessary places - Removed matrix_stack from skia Style class - Moved push_matrix, pop_matrix and style2D class to openglrenderer

Parse input for square

f76a5a2

Parse input for rect() in skia

cd86a10

Update the sanity testing to remove duplicate tests

f39d535

Refactor sanity_testing script

5bf6707

ziyaointl reviewed Jul 12, 2022

View reviewed changes

tushar5526 added 2 commits July 12, 2022 12:35

Refactored vispy renderer

68e928b

Import matrix

409b6d4

ziyaointl reviewed Jul 13, 2022

View reviewed changes

p5/sketch/Vispy2DRenderer/renderer2d.py Outdated Show resolved Hide resolved

tushar5526 added 2 commits July 13, 2022 13:19

Add shape() back to individual renderers

1b81236

Fix tests

57cad00

tushar5526 force-pushed the integrating-skia branch from 0b0d5df to 57cad00 Compare July 13, 2022 08:51

ziyaointl merged commit 6900054 into p5py:master Jul 14, 2022

This was referenced Jul 14, 2022

Enhancing skia further #357

Merged

Add support for environment variables in SkiaSketch #294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrating skia #344

Integrating skia #344

tushar5526 commented Jun 11, 2022 •

edited

Loading

tushar5526 Jun 20, 2022 •

edited

Loading

ziyaointl Jun 25, 2022

tushar5526 Jun 28, 2022

ziyaointl Jun 29, 2022

mrdaybird commented Jun 25, 2022 •

edited

Loading

tushar5526 commented Jun 28, 2022

tushar5526 commented Jul 1, 2022

tushar5526 commented Jul 5, 2022

ziyaointl commented Jul 6, 2022

tushar5526 commented Jul 6, 2022

ziyaointl commented Jul 7, 2022

ziyaointl Jul 10, 2022

tushar5526 Jul 11, 2022

tushar5526 Jul 11, 2022

ziyaointl Jul 12, 2022

ziyaointl Jul 12, 2022

tushar5526 Jul 12, 2022

ziyaointl commented Jul 12, 2022

Integrating skia #344

Integrating skia #344

Conversation

tushar5526 commented Jun 11, 2022 • edited Loading

Environment

Structure

Event handlers

Keyboard

Mouse

Set up the rendering APIs for basic primitive shapes and set up the event handlers using glfw.

tushar5526 Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdaybird commented Jun 25, 2022 • edited Loading

tushar5526 commented Jun 28, 2022

tushar5526 commented Jul 1, 2022

tushar5526 commented Jul 5, 2022

ziyaointl commented Jul 6, 2022

tushar5526 commented Jul 6, 2022

ziyaointl commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyaointl commented Jul 12, 2022

tushar5526 commented Jun 11, 2022 •

edited

Loading

tushar5526 Jun 20, 2022 •

edited

Loading

mrdaybird commented Jun 25, 2022 •

edited

Loading