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

CameraFrame API in the engine #7129

Merged
merged 9 commits into from
Nov 25, 2024
Merged

CameraFrame API in the engine #7129

merged 9 commits into from
Nov 25, 2024

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Nov 21, 2024

Fixes #7129
(separate improvements / changes can be tracked separately)

This adds a new CameraFrame class as part of the engine, which is a public API for the currently exposed render pass based rendering.
The script which exposes this to the Editor now uses an instance of this CameraFrame, and simply passes attributes to it, without containing any logic. This allows it to not access any private engine functionality.

This is the new public API (too long to copy here)
https://github.com/playcanvas/engine/blob/mc-engine-camera-frame/src/extras/render-passes/camera-frame.js

  • also updated engine examples to use CameraFrame module directly, without using the script.
  • updated eslint config to ignore some custom tags we added to MJS scripts

Comment on lines +36 to +42
* @property {string} type - The type of the SSAO determines how it is applied in the rendering
* process. Defaults to {@link SSAOTYPE_NONE}. Can be:
*
* - {@link SSAOTYPE_NONE}
* - {@link SSAOTYPE_LIGHTING}
* - {@link SSAOTYPE_COMBINE}
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally have trouble understanding/visualizing the 3 SSAO types. I would prefer an enabled flag and have it do the most common or nicest looking code path, but provide a further flag along the lines of 'ignoreLighting' or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I improved the descriptions here:
SSAOTYPE_LIGHTING:

  • SSAO is applied during the lighting calculation stage, allowing it to blend seamlessly with scene
  • lighting. This results in ambient occlusion being more pronounced in areas where direct light is
  • obstructed, enhancing realism.

SSAOTYPE_COMBINE:

  • SSAO is applied as a standalone effect after the scene is rendered. This method uniformly
  • overlays ambient occlusion across the image, disregarding direct lighting interactions. While
  • this may sacrifice some realism, it can be advantageous for achieving specific artistic styles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Barging in here, I'd prefer both an enabled property and a type property, defaulting to lighting? I wouldn't be suprised if theres a different ssao code path in the future so a type field might be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we implement different types of SSAO, it would be related to the type of generation of ssao texture.
The current type related to the way ssao texture is consumed by other passes.
So we'd add a different type for that I think.

*/

/**
* @typedef {Object} Bloom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't help but feel that an enabled property per effect would make for a more logical, guessable API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently only TAA and Grading has enabled property. Others have some form of intensity parameter, which disables it when set to 0.
It seems adding enabled there would require two parameters change to use it instead of just a single, so I'm not sure I like that.

@mvaligursky
Copy link
Contributor Author

I'll merge this for now, further feedback / changes will be address by a follow up PR @willeastcott

@mvaligursky mvaligursky merged commit 9efc7d0 into main Nov 25, 2024
8 checks passed
@mvaligursky mvaligursky deleted the mc-engine-camera-frame branch November 25, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants