Should `createCamera` still auto-set itself as the default camera?

[davepagurek]

Topic

When debugging #7071 with @Vishal2002, we noticed a bug in my original bug report's code: I was creating a framebuffer camera in setup with createCamera, and it was accidentally being set as the main canvas's camera. This was happening because the default behaviour of any createCamera() call is not only to return a new camera, but also to basically call setCamera() on it too. If you don't want that, you have to manually surround it with push/pop or an equivalent.

Some thoughts:

1. If you have just one camera, this is probably fine
2. If you switch between multiple cameras, it starts to get a little weird because the last one you create is the default one, unless you do something to prevent that
3. You basically never want this behaviour for framebuffer cameras, since they should only be applied between a framebuffer's begin/end

There are a few ways we could try to resolve this, from lightest to heaviest touch:

• Call out the weird cases in the docs. This is probably not that effective since it's easy to miss even when you know the behaviour.
• 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 each begin/end due to it not saving state between draws.
• Make all cameras not auto-apply themselves. This would perhaps be the most consistent with other creation methods in p5 (they don't usually have side effects), but would also be a breaking change, so this would maybe have to be for 2.0.

[Garima3110]

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 each begin/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();

[Garima3110]

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 each begin/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?!

[davepagurek]

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.

[nickmcintyre]

+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);

[davepagurek]

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.

[nickmcintyre]

@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?

[davepagurek]

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.

[Garima3110]

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.

[davepagurek]

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.