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.

Sign up for GitHub

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

【igl nanovg part-1】shell | mac | add stencil buffer #216

Closed
wants to merge 9 commits into from
Closed

【igl nanovg part-1】shell | mac | add stencil buffer #216

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9e3c99d
shell | mac | add stencil buffer
vinsentli Nov 26, 2024
3ed058e
fix compile error
vinsentli Nov 26, 2024
41fab96
make mac opengl stencil size configurable.
vinsentli Nov 27, 2024
5c7225c
Merge branch 'main' into igl_nanovg_part1_mac_add_stencil_buffer
vinsentli Nov 27, 2024
d861110
const
vinsentli Nov 27, 2024
a80976c
set depth texture format in RenderSession internal.
vinsentli Nov 28, 2024
6d42667
Fix iOS compile error.
vinsentli Dec 3, 2024
df01ffb
Revert "set depth texture format in RenderSession internal."
vinsentli Dec 10, 2024
4188a2c
Revert "iOS : set depth texture format in RenderSession internal."
vinsentli Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion shell/mac/ViewController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ - (void)render {
}

- (void)loadView {
session_ = factory_->createRenderSession(nullptr);

// We don't care about the device type, just
// return something that works
HWDeviceQueryDesc queryDesc(HWDeviceType::Unknown);
Expand Down Expand Up @@ -196,6 +198,10 @@ - (void)loadView {

#if IGL_BACKEND_OPENGL
case igl::BackendFlavor::OpenGL: {
const bool enableStencilBuffer = session_->getDepthTextureFormat() == igl::TextureFormat::S8_UInt_Z24_UNorm ||
session_->getDepthTextureFormat() == igl::TextureFormat::S_UInt8;
const NSOpenGLPixelFormatAttribute stencilSize = enableStencilBuffer ? 8 : 0;

NSOpenGLPixelFormat* pixelFormat;
if (config_.backendVersion.majorVersion == 4 && config_.backendVersion.minorVersion == 1) {
static NSOpenGLPixelFormatAttribute attributes[] = {
Expand All @@ -211,6 +217,8 @@ - (void)loadView {
32,
NSOpenGLPFADepthSize,
24,
NSOpenGLPFAStencilSize,
stencilSize,
NSOpenGLPFAOpenGLProfile,
NSOpenGLProfileVersion4_1Core,
0,
Expand All @@ -232,6 +240,8 @@ - (void)loadView {
32,
NSOpenGLPFADepthSize,
24,
NSOpenGLPFAStencilSize,
stencilSize,
NSOpenGLPFAOpenGLProfile,
NSOpenGLProfileVersion3_2Core,
0,
Expand All @@ -252,6 +262,8 @@ - (void)loadView {
32,
NSOpenGLPFADepthSize,
24,
NSOpenGLPFAStencilSize,
stencilSize,
NSOpenGLPFAOpenGLProfile,
NSOpenGLProfileVersionLegacy,
0,
Expand Down Expand Up @@ -341,7 +353,7 @@ - (void)loadView {
}
}

session_ = factory_->createRenderSession(shellPlatform_);
session_->setPlatform(shellPlatform_);
IGL_DEBUG_ASSERT(session_, "createDefaultRenderSession() must return a valid session");
// Get initial native surface dimensions
shellParams_.nativeSurfaceDimensions = glm::ivec2(2048, 1536);
Expand Down
9 changes: 9 additions & 0 deletions shell/shared/renderSession/RenderSession.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ class RenderSession {
// NOLINTNEXTLINE(performance-unnecessary-value-param)
virtual void update(IGL_MAYBE_UNUSED igl::SurfaceTextures surfaceTextures) noexcept {}
virtual void teardown() noexcept {}

void setPlatform(std::shared_ptr<Platform> platform){
platform_ = std::move(platform);
}

void updateDisplayScale(float scale) noexcept;

Expand All @@ -45,6 +49,10 @@ class RenderSession {
void setCurrentQuadLayer(size_t layer) noexcept {
currentQuadLayer_ = layer;
}

[[nodiscard]] igl::TextureFormat getDepthTextureFormat() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this addition? The point of suggested+requested configs is to let a particular session choose the formats it wants without needing to create some kind of dummy session without a platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You want the stencil buffer to be disabled by default. So, I can only move the depthTextureFormat field from RenderSessionConfig to RenderSession, and then NanovgRenderSession can override the default depthTextureFormat.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anything already merged into main is fine. For any further changes, aim to make only the minimal changes necessary. Please update your diffs to minimize the modifications required to add NanoVG.

Copy link
Contributor Author

@vinsentli vinsentli Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The minimal way to make the changes is to keep the depthTextureFormat field in RenderSessionConfig.

If I want to show the NanoVG session and make changes to depthTextureFormat locally, and avoid committing it to the main branch.

I can add comments in NanovgSession to explain that other developers can make such changes if they want to show the effect.

This approach is also well-suited for Android， because without it, the changes required for Android would be significant as well.

What do you think about this approach?

return depthTextureFormat_;
}

/// return the number of seconds since the last call
float getDeltaSeconds() noexcept;
Expand Down Expand Up @@ -74,6 +82,7 @@ class RenderSession {

AppParams& appParamsRef() noexcept;

igl::TextureFormat depthTextureFormat_ = igl::TextureFormat::Z_UNorm16;
std::shared_ptr<IFramebuffer> framebuffer_;
std::shared_ptr<ICommandQueue> commandQueue_;
size_t currentQuadLayer_ = 0;
Expand Down
1 change: 0 additions & 1 deletion shell/shared/renderSession/RenderSessionConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ struct RenderSessionConfig {
std::string displayName;
BackendVersion backendVersion;
igl::TextureFormat swapchainColorTextureFormat = igl::TextureFormat::BGRA_UNorm8;
igl::TextureFormat depthTextureFormat = igl::TextureFormat::Z_UNorm16;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of our internal code that relies on this field. Is it possible to add stencil buffer support without getting rid of it?

Copy link
Contributor Author

@vinsentli vinsentli Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The field depthTextureFormat is add by me in this commit.49d674c

This PR revert the commit before, add move the depthTextureFormat field into RenderSession.

Copy link
Contributor

Choose a reason for hiding this comment

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

The field depthTextureFormat is add by me in this commit.49d674c

I looked into it, the tests do not pass because the file shell/ios/ViewController.mm needs to be updated as well. It still uses depthTextureFormat.

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 have fixed iOS compile error.

The complete adaptation of the iOS stencil buffer is another PR(#221).
It should be merged with the latest main branch and reviewed after this PR is approved.

igl::ColorSpace swapchainColorSpace = igl::ColorSpace::SRGB_NONLINEAR;
};

Expand Down
Loading