processing / p5.js

p5.js is a client-side JS platform that empowers artists, designers, students, and anyone to learn to code and express themselves creatively on the web. It is based on the core principles of Processing. http://twitter.com/p5xjs —
http://p5js.org/
GNU Lesser General Public License v2.1
21.7k stars 3.34k forks source link

Confusion between .elt and .canvas #3156

Open Zalastax opened 6 years ago

Zalastax commented 6 years ago

@Spongman highlights in #3155 that there is a confusion between the properties .elt and .canvas causing us to write defensive code like so: https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/src/core/p5.Renderer2D.js#L112

What is the purpose of having both .elt and .canvas? How can it be that the canvas should sometimes come from .elt and sometimes from .canvas?

Searching for .canvas = I get these results: https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/lib/addons/p5.dom.js#L2637 https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/src/core/p5.Graphics.js#L29 https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/src/core/p5.Renderer.js#L26 https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/src/image/p5.Image.js#L144 https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/src/webgl/p5.RendererGL.js#L176 Searching for .elt = I get these results: https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/src/core/p5.Element.js#L44 https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/lib/addons/p5.dom.js#L585 https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/lib/addons/p5.dom.js#L723 and a comment about checking if the canvas is associated with the p5 instance https://github.com/processing/p5.js/blob/5a46133fdc3e8c42fda1c1888864cf499940d86d/lib/addons/p5.dom.js#L1938-L1939

Some of the places that assign canvas also assign elt afterwards, some do not. In #3155 @Spongman solves the problem via if (!this.canvas) this.loadPixels();, is that a general approach we should apply elsewhere? Do we have a place where we can add (or already have?) YUIDocs for .elt and .canvas?

Spongman commented 6 years ago

the solution to the #3155 issue is unrelated to this. p5.MediaElement does actually require both .elt and .canvas properties - the .elt is the video element, and the .canvas is the bitmap cache that supports get & set.

the issue here is that p5.Element defines a .elt property, p5.Graphics, p5.Renderer derive from p5.Element (so inherit the .elt property), but they also define their own .canvas property and much of the code assumes the existence of .canvas, however sometimes those methods are used to act on p5.Image objects that (being p5.Elements) only define .elt.

IMO, (except for p5.MediaElement) those canvas properties should be removed and code referencing them should instead refer to .elt.

ok, it's a little more complicated than that. MediaElement is an Element, but MediaElement pretends to be a Renderer when doing its pixels stuff which means there's a conflict between canvas and elt in MediaElement - they're not the same thing. so doing the above would break MediaElement, again.

Zalastax commented 6 years ago

Thanks for the clarification!

Is this correct? We have the class p5.Element which is extended and an interface Renderable

interface Renderable {
  canvas: HTMLCanvasElement
}

Some functions accept parameters of type p5.Element | Renderable. A parameter that is both p5.Element and Renderable will be seen only as a Renderable (cnv = img.canvas || img.elt;).

What is the type for .elt? Any HMLElement but sometimes assumed to be HTMLCanvasElement? Are there any actions we can and should take?

Spongman commented 6 years ago

it does look a bit like that, yes. although instead of those functions accepting parameters of different types, they're invoked with this being objects of different types. just a syntactic difference.

i'm not sure there's anything short of major architectural reworking that can be done to fix this right now.

but... my pie-in-the-sky idea: instead of p5.Graphics 'borrowing' methods from p5, and p5.Image 'borrowing' methods from p5.Renderer, maybe things could be turned around a little to end up with an actual inheritance chain that looks something like this:

the API would mostly stay the same except for the following: