Open davepagurek opened 4 weeks ago
There are a few ways we could try to resolve this, from lightest to heaviest touch:
- Make framebuffer cameras not auto-apply themselves. This would resolve point 3 without really being a breaking change, because it would have caused buggy behaviour before if you were creating a framebuffer camera outside of
push
/pop
. It introduces a bit of inconsistency, but the way you use cameras in framebuffers is already a bit inconsistent because you have to apply a camera within eachbegin
/end
due to it not saving state between draws.
In view of this @davepagurek maybe we can introduce a new parameter or function option for createCamera()
to control whether it sets itself as the default camera. For example, createCamera(autoSetDefault = true)
. This allows for backward compatibility while giving users more control.
Its implementation example can be something like:
// Current behavior
let mainCamera = createCamera(); // Sets itself as default
// Suggested new parameter approach
let mainCamera = createCamera(true); // Explicitly sets itself as default
let framebufferCamera = createCamera(false); // Does not set itself as default
// Usage with push/pop
push();
setCamera(framebufferCamera);
// Perform framebuffer operations
pop();
There are a few ways we could try to resolve this, from lightest to heaviest touch:
- Make framebuffer cameras not auto-apply themselves. This would resolve point 3 without really being a breaking change, because it would have caused buggy behaviour before if you were creating a framebuffer camera outside of
push
/pop
. It introduces a bit of inconsistency, but the way you use cameras in framebuffers is already a bit inconsistent because you have to apply a camera within eachbegin
/end
due to it not saving state between draws.In view of this @davepagurek maybe we can introduce a new parameter or function option for
createCamera()
to control whether it sets itself as the default camera. For example,createCamera(autoSetDefault = true)
. This allows for backward compatibility while giving users more control.Its implementation example can be something like:
// Current behavior let mainCamera = createCamera(); // Sets itself as default // Suggested new parameter approach let mainCamera = createCamera(true); // Explicitly sets itself as default let framebufferCamera = createCamera(false); // Does not set itself as default // Usage with push/pop push(); setCamera(framebufferCamera); // Perform framebuffer operations pop();
@davepagurek Please let me know if this approach seems fine to you, and if you agree may be I could go on with making a PR for this?!
Hi, sorry for the delay! I think making it by default not set itself, and letting you optionally make it set itself would be a good change. Maybe to make it easier to read the code, we could have an options object instead of just a boolean parameter? Something like:
myCam = createCamera({ setDefault: true })
The other thing is, as this would be a change to default behaviour, we'd probably need to branch off of the dev-2.0 branch rather than the main branch to make the change just be released as part of 2.0.
+1 for the "Make all cameras not auto-apply themselves" approach. What do people think about matching createCamera()
's parameters with camera()
?
// createCamera([x], [y], [z], [centerX], [centerY], [centerZ], [upX], [upY], [upZ])
let myCam = createCamera(200, -400, 800);
setCamera(myCam);
I think that could make sense @nickmcintyre. What are your thoughts on having the optional ability to set a camera as default? Would it be worth adding an optional options object to the end of that signature, or just relying on setCamera()
? I can see an argument for the explicit setCamera
being more readable.
@davepagurek @Garima3110 I think I'm in favor of using setCamera()
for clarity. Are there other options that are likely to be included in the options object?
Not currently I think, I had suggested it to avoid the slightly more cryptic API of createCamera(true)
or createCamera(false)
.
Since we already have setCamera
, it's maybe less confusing to just have the one way to do it rather than two, so I think my preference also leans toward just using that and not adding options to createCamera
, but I don't have a super strong opinion here.
Since we already have setCamera, it's maybe less confusing to just have the one way to do it rather than two
Considering your points, @davepagurek and @nickmcintyre, I agree that using setCamera()
is preferable to adding an options object to createCamera()
.
As mentioned in the discussion, having options like createCamera(true)
or even createCamera({ default: true })
can be cryptic and less intuitive compared to the explicit setCamera()
. This explicit approach maybe (not exactly sure though) would enhance readability and clarity but would also ensure that the API remains accessible to beginners in the p5 community.
I think we're ok to make a PR into the dev-2.0 branch then if you're up for it! For all 2.0 features, we're going to have an advisory committee help make the final decision on what gets included, but the dev-2.0 branch is where we've been working on things we feel reasonably confident in and wanted to start prototyping.
Topic
When debugging https://github.com/processing/p5.js/issues/7071 with @Vishal2002, we noticed a bug in my original bug report's code: I was creating a framebuffer camera in
setup
withcreateCamera
, and it was accidentally being set as the main canvas's camera. This was happening because the default behaviour of anycreateCamera()
call is not only to return a new camera, but also to basically callsetCamera()
on it too. If you don't want that, you have to manually surround it withpush
/pop
or an equivalent.Some thoughts:
begin
/end
There are a few ways we could try to resolve this, from lightest to heaviest touch:
push
/pop
. It introduces a bit of inconsistency, but the way you use cameras in framebuffers is already a bit inconsistent because you have to apply a camera within eachbegin
/end
due to it not saving state between draws.