-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Support for 3D coordinate on Image #7172
base: main
Are you sure you want to change the base?
Conversation
@davepagurek , please have a look at my PR. Is it in the right direction ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on! I think we'll have to do some updates in order to make sure 2D image()
still works. Let me know what you think!
@@ -1097,6 +1097,7 @@ p5.prototype.image = function( | |||
img, | |||
dx, | |||
dy, | |||
dz, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think placing this argument here will break calls to image()
that don't use z
but do use the arguments that follow it.
I think we might need to follow the approach of some other 2D/3D functions like dist
here, where we reinterpret the arguments array based on some criteria:
Lines 221 to 227 in 21d7c8c
if (args.length === 4) { | |
//2D | |
return Math.hypot(args[2] - args[0], args[3] - args[1]); | |
} else if (args.length === 6) { | |
//3D | |
return Math.hypot(args[3] - args[0], args[4] - args[1], args[5] - args[2]); | |
} |
In this case, the criteria will probably depend on the length of the arguments.
Since this is somewhat complex, I think it would also be a good idea to add some unit tests to make sure that this continues to work for the 2D case as well as the new 3D case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review. I went with the length approach initially .
Odd number of parameters → Use 2D mode.
Even number of parameters → Check if the 10th parameter is a number.
If it's not a number, defer to 2D mode.
If it's a number, use 3D mode.
but starting from the third parameter single arguments become optional image(img, dx, dy, dWidth, dHeight, sx, sy, [sWidth], [sHeight], [fit], [xAlign], [yAlign])
hence my even/odd length logic fails as soon as the function starts receiving optional arguments.
The best solution I got at the moment was, to shift the values of the input argument to the right in case of 2D_rendering.
if(this._renderer instanceof p5.RendererGL == false){
// Store the value of 'yAlign' temporarily
let temp = yAlign;
// From the 3rd arguement shift the assingment one position to the right
yAlign = xAlign;
xAlign = fit;
fit = sHeight;
sHeight = sWidth;
sWidth = sy;
sy = sx;
sx = dHeight;
dHeight = dWidth;
dWidth = dz;
}
The results have been successful in every test .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also work for 3D mode if the z coordinate is omitted?
Also a possible other approach for the even/odd check: you might need to check if the number of arguments up to the fit
parameter (which should be a string instead of a number) is even or odd. That might mean something like:
let endIndex = args.findIndex(arg => arg === constants.COVER || arg === constants.CONTAIN)
if (endIndex === -1) {
endIndex = args.length
}
const argsUpToFit = args.slice(0, endIndex)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also work for 3D mode if the z coordinate is omitted?
yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are in WebGL mode and call image
without z
but with some of the options after z
, does that still work? e.g. this should draw the image fullscreen on the canvas:
imageMode(CENTER)
image(yourImg, 0, 0, width, height, 0, 0, yourImg.width, yourImg.height, COVER)
Just based on the code so far, it looks like in WebGL mode, it never shifts all the arguments over the way it does when this._renderer instanceof p5.RendererGL === false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2D sample code let img;
function preload() {
// Load the image
img = loadImage('https://upload.wikimedia.org/wikipedia/commons/thumb/6/69/June_odd-eyed-cat_cropped.jpg/712px-June_odd-eyed-cat_cropped.jpg');
}
function setup() {
createCanvas(800, 800 , WEBGL); // Create a 3D canvas
background(200);
}
function draw() {
background(200);
// Second image at another position
image(img, 100, 100,0); // x, y, z coordinates
} Every thing seems fine by my knowledge. Please for a hint as to why this problem occurs. |
Just to confirm, do you normalize the starting coordinate between WebGL and 2D modes, e.g. by doing |
Thank you Dave . For 3D mode , It is better to draw texture with let img;
function preload() {
img = loadImage('https://upload.wikimedia.org/wikipedia/commons/thumb/6/69/June_odd-eyed-cat_cropped.jpg/712px-June_odd-eyed-cat_cropped.jpg');
}
function setup() {
createCanvas(800, 800, WEBGL); // Create a 3D canvas
background(200);
}
function draw() {
background(200);
image(img, 100, 100, 0);
} |
But when I introduce |
The parameter checking is done based on the documentated parameter types, and by the looks of it, currently we've added |
src/webgl/p5.RendererGL.js
Outdated
@@ -2391,6 +2391,44 @@ p5.RendererGL = class RendererGL extends p5.Renderer { | |||
|
|||
return triangleVerts; | |||
} | |||
|
|||
image3D(img,sx,sy,sz,sWidth,sHeight,dx,dy,dWidth,dHeight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shares a lot of code with p5.RendererGL.image
, maybe we could make that function support a z coordinate rather than duplicating the code for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses 3D primitives
and changes to p5.RendererGL
has been removed
src/image/loading_displaying.js
Outdated
); | ||
//if it is not graphics nor framebuffer but WEGL instance | ||
//default to use 3D rendering | ||
if (this._renderer instanceof p5.RendererGL && !isP5G && !isP5Fbo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like maybe this check should be a check for whether the dz
parameter exists rather than the type of the image? since you could plausibly be drawing a graphic or a framebuffer both in 3D or without specifying a z
@sproutleaf , sorry to bother you . Could you give me some hints or direction, I realize that when I add one more parameter (dz) to a function , I am caught with a friendly error message and failed tests. Please could you help me with a few hints on how to address this. |
@@ -1110,7 +1213,20 @@ p5.prototype.image = function( | |||
// set defaults per spec: https://goo.gl/3ykfOq | |||
|
|||
p5._validateParameters('image', arguments); | |||
|
|||
// If dz is not specified. | |||
if (arguments.length % 2 === 1 || typeof arguments[9] === 'string'){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not completely sure if this is causing the test to fail, but I believe the code enters the if block in both cases—whether dz is specified or not.
For example, when you check arguments.length, if z is provided it comes out to be an odd number, and arguments[9] will be a number.
However, if the dz variable isn't specified, arguments.length will be even, and in this case, arguments[9] will be a string.
In both cases I am talking about this example:
image(yourImg, 0, 0, width, height, 0, 0, yourImg.width, yourImg.height, COVER)
when z is not specified.
If you do something like:
if(arg.length%2 !== 1 || typeof arg[9] ==='string){...}
is that works for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I’m concerned that we might need a different approach for other cases. For example, if we have a function call like image(img, 100, 100, 0);
, the number of arguments is four, we have arg.length % 2=== 1
for which z is not specified.
We could probably have a general case for all the cases which could work for image(no. of arguments).
let me know what you think?
Hi @Forchapeatl , I apologize for the delayed response. The maintainers have been very busy with the version update to 2.0. I can help you figure out what's going on. It's looking great so far! Do you mind sharing the sketch where you get friendly errors? thanks for your work on this:) |
Resolves #5861
Changes:
Screenshots of the change:
PR Checklist
npm run lint
passesExample code