push_matrix() and pop_matrix() not working

[tyagi619]

using pop_matrix() does not restore the transform matrix to the previous state. I tried using the exact codes mentioned in your documentation : https://p5.readthedocs.io/_/downloads/en/latest/pdf/.

from p5 import *
def setup():
size(200, 200)
def draw():
background(255)
fill(192)
no_stroke()
rect((40, 40), 40, 40)
push_matrix()
translate(40, 40)
rotate(radians(45))
fill(0)
rect((40, 40), 40, 40)
pop_matrix()
if name == 'main':
run()

The following code clearly depicts the error

[tyagi619]

The syntax in the tutorials is incorrect. Instead of:
push_matrix()
.... Some Code ....
pop_matrix()

It should be:
with push_matrix():
....Some transformation......

[jeremydouglass]

The following code clearly depicts the error

I can't reproduce this. When I run the above code, with proper indenting, and with a correct __name__, like this:

from p5 import *

def setup():
  size(200, 200)
  
def draw():
  background(255)
  fill(192)
  no_stroke()
  rect((40, 40), 40, 40)
  push_matrix()
  translate(40, 40)
  rotate(radians(45))
  fill(0)
  rect((40, 40), 40, 40)
  pop_matrix()

if __name__ == '__main__':
    run()

I get:

That is correct (equivalent to output in Processing, Processing.py, et cetera)

[tyagi619]

I kind of made a mistake while copying the code. Here is the code:
from p5 import *
def setup():
size(200, 200)
def draw():
background(255)
fill(192)
rect((40, 40), 40, 40)
push_matrix()
translate(40, 40)
rotate(radians(45))
fill(0,255,0,128)
rect((0, 0), 40, 40)
pop_matrix()
rect((0, 0), 40, 40)
if __name__ == '__main__':
run()

I don't know how to add indentation to code here.

This is the output in p5:

This is the output in processing, processing.py:

[jeremydouglass]

Format you code by highlighting it and pressing <> button. This puts three ` above and below.

Your second code does show an error (although your "this is the output in processing, processing.py" is incorrect, it doesn't matter).

I can reproduce a problem with pop_matrix() like this.

from p5 import *

def setup():
  size(200, 200)

def draw():
  background(255)
  translate(40, 40)
  
  fill(0)
  rect((0, 0), 80, 40)
  
  # translate and rotate
  push_matrix()
  translate(40, 40)
  rotate(radians(45))
  fill(128)
  rect((0, 0), 80, 30)
  pop_matrix()
  
  # red shape should be on first
  # at 40,40
  # but is on second
  # at 80,80 r45
  fill(255,0,0)
  rect((0, 0), 80, 20)
  
if __name__ == '__main__':
  run()

[tyagi619]

Can you share the result for your code in processing? I would really like to know what did i do wrong while writing in processing.py. It would be nice if you could share the code as well as the output

[jeremydouglass]

Sure, but then we should move this discussion to the forum:

https://discourse.processing.org

GitHub issues aren't the best place for coding help discussions.

The changes to make are:

1. delete import
2. delete if __name__
3. change (x, y) tuples to separate arguments: rect(40, 40, 40, 40)
4. change push_matrix() to pushMatrix(), (and pop_matrix() the same)

def setup():
  size(200, 200)

def draw():
  background(255)
  fill(192)
  rect(40, 40, 40, 40)
  pushMatrix()
  translate(40, 40)
  rotate(radians(45))
  fill(0,255,0,128)
  rect(0, 0, 40, 40)
  popMatrix()
  rect(0, 0, 40, 40)

As you can see, the third rect is green. In your drawing it is gray. I'm not sure what you did.

If you want to write idiomatic processing.py, you can also make pushMatrix() a self-closing context manager using with and dropping popMatrix() like this:

def setup():
  size(200, 200)

def draw():
  background(255)
  fill(192)
  rect(40, 40, 40, 40)
  with pushMatrix():
    translate(40, 40)
    rotate(radians(45))
    fill(0,255,0,128)
    rect(0, 0, 40, 40)
  rect(0, 0, 40, 40)

This produces the same image output as above.

[tyagi619]

As far as the issue is concerned, using

with push_matrix():
        ....Some Code.....

does solve the issue

[jeremydouglass]

does solve the issue

Huh. It produces correct behavior, yes.

But the issue you found is pop_matrix() not working -- it doesn't solve that issue.

[jeremydouglass]

Hmm. Looking at the documentation here: https://github.com/p5py/p5/blame/master/docs/guides/for-processing-users.rst

• Processing's :code:pushMatrix() and :code:popMatrix() have been
  replaced by a single :code:push_matrix() context manager that
  cleans up after itself. [...]

     with push_matrix():
         translate(width / 2, height / 2)
         point(0, 0)

In processing.py pushMatrix() can be used either as a context manager() or to push with a manual pop, in the Java style.

If manual push_matrix() in p5py isn't implemented in the non-context-manager way, it looks like the issue might be that pop_matrix() should be dropped entirely from the api or throw an error.

[jeremydouglass]

See #147

[tyagi619]

i went through the code and this is how pop_matrix is implemented:

def pop_matrix():
        push_matrix()

As you pointed out it needs to be dropped as it seems to be redundant
Going through the code, I figured out, pop_style has the same issue. Here is the implementation of pop_style:

def pop_style():
        push_style()

push_style is also implemented as a context-manager, so it will be used as follows:

with push_style():
        ...... Some Code.....

[jeremydouglass]

Is this true of push() and pop() in the same way? Should pop() be dropped from documentation, and push() only used with with push(): ?

Edit Never mind, looks like push and pop aren't implemented -- I was seeing only seeing them in code search due to a javascript web preview.

[tyagi619]

Can i fix this issue?

[jeremydouglass]

Hi @tyagi619 -- could you test / code review the PR that I posted? Unless you suggest a different approach?

[tyagi619]

i guess i can

[jeremydouglass]

Did you want to do something else instead, or your own PR?

[jeremydouglass]

@tyagi619 -- would you prefer that I withdraw my PR so that you can submit your own?

[tyagi619]

that would be great

[tyagi619]

We can use a stack to store the transformation matrix whenever push_matrix() is called and then pop from the stack whenever pop_matrix() is called. Would it be better this way or should I just remove the pop_matrix() ?

[jeremydouglass]

I think just remove pop_matrix, and clean up the reference and documentation accordingly, as per:

Processing's pushMatrix() and popMatrix() have been replaced by a single push_matrix() context manager that cleans up after itself. https://github.com/p5py/p5/blob/master/docs/guides/for-processing-users.rst

This was what I modeled in my PR and originally requested your review.

@parsoyaarihant @abhikpal -- do you think p5 should implement both with push_matrix( ): AND push_matrix() ... pop_matrix(), the way that Processing.py does, or do you want to only support with push_matrix(): and remove pop_matrix(), as described in the docs?

Note that using pop_matrix as a context manager as implemented already creates a de-facto stack -- the call stack. You can see recursively nested calls creating a matrix stack ~5-deep, here:

from p5 import *

def setup():
    size(400,400)
    no_loop()

def draw():
    background(255)
    translate(width/2, height/2)
    quad(width)
    no_loop()

def quad(w):
    w = w*0.8;
    if w < 5: return
    fill(255,random_uniform(255),random_uniform(255))
    rect((-w/2,-w/2),w,w)
    with push_matrix():
        translate(-w/4,-w/4);
        quad(w/2)
    with push_matrix():
        translate(w/4,-w/4);
        quad(w/2)
    with push_matrix():
        translate(-w/4,w/4);
        quad(w/2)

if __name__ == '__main__':
    run()

[tyagi619]

Just let me know which way you would prefer and i would do it accordingly
@jeremydouglass

[jeremydouglass]

Great @tyagi619 -- since we haven't heard back from @parsoyaarihant @abhikpal, I'd suggest going ahead with:

make a PR supporting with push_matrix(): only, removing pop_matrix()

I believe this approach is consistent with their design and documentation.

[tyagi619]

@jeremydouglass i will do the updates by tomorrow

[jeremydouglass]

Hi @tyagi619 -- are you still planning on opening a PR for this?