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

Refactored Vispy2D renderer under one module #288

Merged
merged 10 commits into from
Oct 31, 2020

Conversation

tushar5526
Copy link
Member

@tushar5526 tushar5526 commented Oct 17, 2020

Userspace.py now only imports and uses vispy as renderer when selected.
Base.py and renderer2D.py is refactored.

Before working on integrating Cairo and refactoring code, should we work on writing visual tests for p5py?
As sooner or later we will need that and manually checking for bugs when we will start integrating Cairo will become hectic.

This PR is not tested robustly, I just tested it on some 2D and 3D examples, and things were working fine :)

…ispy

Refactored userspace.py to import vispy as 2D renderer only when selected

app.run()
exit()
app.run()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is visy specific. I believe if we use another renderer, it will throw an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

app.run() will be executed only when the renderer is set to vispy.
app.run() and all other vispy specific calls is under the if renderer == "vispy" clause. I think Github's UI is making it confusing to figure out the indents.

Or is it something else that I am missing, can you confirm this @parsoyaarihant ?
Thank you for the review :)

@arihantparsoya
Copy link
Member

To test the changes, we can directly checkout your branch locally and test it. We dont have to create a new branch in p5py.

@arihantparsoya
Copy link
Member

Nice work! I will test this properly in the coming days.

For #214 we now need two things:

  1. Refactor the PShape class into the newly created vispy renderer. Basically move the shape.py file into the Vispy2DRenderer folder.
  2. Create new methods in Vispy2DRenderer class for the primitive APIs (like line, rectangle, etc). Then call these functions from the primitives.py. The primitive shape functions should not return a PShape object like the currently do, instead they should call the corresponding function in the Vispy2DRenderer.

@arihantparsoya
Copy link
Member

Can you put shaders2d.py (and shader files) and openglrenderer inside the Vispy2DRenderer?

@tushar5526
Copy link
Member Author

tushar5526 commented Oct 18, 2020

  1. Refactor the PShape class into the newly created vispy renderer. Basically move the shape.py file into the Vispy2DRenderer folder.
  2. Create new methods in Vispy2DRenderer class for the primitive APIs (like line, rectangle, etc). Then call these functions from the primitives.py. The primitive shape functions should not return a PShape object like the currently do, instead they should call the corresponding function in the Vispy2DRenderer.

Thanks for the detailed description of what needs to be done. I will start working on it, but due to exams will be little inactive till the month's end.

… to 3D vispy renderer

Moved Shader files in Vispy2DRenderer
Userspace.py and utli.py refactored to support the new changes
@tushar5526
Copy link
Member Author

Now, renderer2d and renderer3d is imported after we check which renderer mode and renderer backend is specified by the user.
I have also encapsulated all 3d renderer code ( shader files, renderer3d.py) in Vispy3DRenderer, otherwise renderer3d was loading shader files before builtins.current_renderer was defined.
I tried to generalize things as much as possible so that it's easy to add other backends for both 2D and 3D in the future.

.gitignore Outdated
@@ -111,3 +111,8 @@ ENV/

# Profiling results
profiling/*.prof

.idea/
test.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useful for development. But we need to discuss and document this properly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have documented this.

@@ -25,7 +25,7 @@
from ..pmath import curves
from ..pmath.utils import SINCOS

from .shape import PShape
from p5.sketch.Vispy2DRenderer.shape import PShape
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont have to use the entire namespace. We can use from ..sketch.Vispy2DRenderer.shape import PShape

Copy link
Member Author

@tushar5526 tushar5526 Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes in namespaces were done automatically by pycharm when I moved (refactored) the files. I will change it to relative imports then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix these imports. Then I will merge this PR

@tushar5526
Copy link
Member Author

@parsoyaarihant before I redo the APIs, can you confirm is this the intended design we are looking for?

def quad(*args):
    """doc string"""
    
    (parse args here)
    if len(args) == 8:
        p1, p2, p3, p4 = args[:2], args[2:4], args[4:6], args[6:]
    elif len(args) == 4:
        p1, p2, p3, p4 = args
    else:
        raise ValueError("Unexpected number of arguments passed to quad()")
    
    (pass onto renderer then)
    p5.renderer.quad(p1, p2, p3, p4)

Quad method in VispyRenderer Class

    def quad(self, *args):
        p1, p2, p3, p4 = args
        path = [
            Point(*p1),
            Point(*p2),
            Point(*p3),
            Point(*p4)
        ]
        self.render(PShape(vertices=path, shape_type=SType.QUADS))

@ziyaointl
Copy link
Member

Great work! Now that the shaders have moved, you may want to modify MANIFEST.in to include the new shader location.

@arihantparsoya
Copy link
Member

@parsoyaarihant before I redo the APIs, can you confirm is this the intended design we are looking for?

Yes, this is the design we are looking for

@arihantparsoya
Copy link
Member

Before working on integrating Cairo and refactoring code, should we work on writing visual tests for p5py?

Cairo has anti-aliasing and Vispy does not. If we write the tests for Vispy, then they will fail with Cairo.

@tushar5526
Copy link
Member Author

I have compiled all the examples present at p5's read the docs for testing purposes. In case you need it, they are under check_tests at this branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants