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

setAttributes() invalidates references to earlier canvases #5902

Open
1 of 17 tasks
davepagurek opened this issue Dec 16, 2022 · 7 comments
Open
1 of 17 tasks

setAttributes() invalidates references to earlier canvases #5902

davepagurek opened this issue Dec 16, 2022 · 7 comments

Comments

@davepagurek
Copy link
Contributor

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Build Process
  • Unit Testing
  • Internalization
  • Friendly Errors
  • Other (specify if possible)

p5.js version

1.5.0

Web browser and version

107.0.1

Operating System

macOS 12.5.1

Steps to reproduce this

Calling setAttributes in WebGL mode might end up recreating the underlying canvas and renderer object.

Say you're trying to position the element, e.g. like this:

let canvas = createCanvas(100, 100, sketch.WEBGL);
// ...
canvas.position(0,0);

Unfortunately, this stops working after a setAttributes call, because after that call, canvas !== _renderer. Currently, to fix it, one would have to do:

createCanvas(100, 100, sketch.WEBGL);
setAttributes({ alpha: true });
let canvas = _renderer;
// ...
canvas.position(0,0);
@Ucodia
Copy link
Contributor

Ucodia commented Feb 22, 2023

Was able to reproduce the issue. When setAttributes() is called, the canvas context is reset, which recreates the renderer, thus the initial reference returned by createCanvas() points to the old renderer.

See code:

Calling new p5.RendererGL() is the core of the issue here. Now I don't know how this can be fixed in the library as I don't have experience with WebGL. What I know is that attributes have to be passed to canvas.getContext(), meaning that any change to these attributes requires a recreation of the context, this is a limitation of the HTMLCanvasElement API.

In the meantime, the safest method is to always refer to the renderer from its internal pointer this._renderer rather than the instance returned from createCanvas().

@davepagurek
Copy link
Contributor Author

A fix might require a slight redesign of what gets returned by createCanvas. Maybe it could return a thin wrapper over p5.Renderer, so that when we make a new renderer, we can also update the canvas object to have the new renderer? When createCanvas returns the renderer directly, it's a little hard to replace with a totally new renderer.

@Artimus100
Copy link
Contributor

hello @davepagurek this seems interesting too ! Could you explain this or maybe elaborate on what needs to be done here
lets see if i can be of any help!

-_- Thank You

@Artimus100
Copy link
Contributor

@davepagurek Should i start working on this?

@davepagurek
Copy link
Contributor Author

@Artimus100 feel free to start looking into it, but this one probably could use some discussion here in the issue once you have an idea of how to solve it before making a PR.

The problem under the hood is that the canvas created by createCanvas is a p5.RendererGL. Changing the attributes of a canvas requires us to recreate the canvas. That means that the return value of the original createCanvas call will no longer refer to the real canvas after calling setAttributes because we've made a new one and set this._renderer to the new value.

I mentioned in a comment that one way to address the issue is, rather than directly returning the renderer from createCanvas, we instead return an object that keeps a reference to the p5 instance, and when you call e.g. .position(x, y) on it, it internally calls p5Instance._renderer.position(x, y), since the _renderer property will point to the up-to-date canvas. Maybe this could be done via a Proxy object? There are also other ways to implement it too, and potentially other designs that work differently. I think the next step would be to look into what it would take to implement an idea and propose it so we can think about whether the design would fully solve the problem.

@Forchapeatl
Copy link
Contributor

@Artimus100 , @davepagurek can I take this up ? I would love to look into this.

@davepagurek
Copy link
Contributor Author

Thanks @Forchapeatl! Let me know if you have any ideas on approaches to take here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Bugs with No Solution Yet
Development

No branches or pull requests

5 participants