【igl nanovg part-1】shell | mac | add stencil buffer by vinsentli · Pull Request #216 · facebook/igl

vinsentli · 2024-11-26T03:02:12Z

This is a prerequisite pull request for igl nanovg(#213).

corporateshark

Please make it configurable, with the default setting matching the previous behavior. No stencil buffer should be enabled by default unless requested explicitly.

vinsentli · 2024-11-27T11:00:24Z

Please make it configurable, with the default setting matching the previous behavior. No stencil buffer should be enabled by default unless requested explicitly.

done

corporateshark · 2024-11-27T11:01:45Z


 #if IGL_BACKEND_OPENGL
  case igl::BackendFlavor::OpenGL: {
+    bool enableStencilBuffer = config_.depthTextureFormat == igl::TextureFormat::S8_UInt_Z24_UNorm  ||


corporateshark

The default behavior should be to keep the stencil buffer disabled.

corporateshark · 2024-11-27T19:27:27Z

                             .majorVersion = 3,
                             .minorVersion = 0},
          .swapchainColorTextureFormat = kColorFramebufferFormat,
+          .depthTextureFormat = igl::TextureFormat::S8_UInt_Z24_UNorm,


This should not ne changed for all apps. Only apps using NanoVG would want to enable the stencil buffer.

This line still stands. We should not enable the stencil buffer unconditionally for all apps.

vinsentli · 2024-11-28T03:42:47Z

I have moved the depth texture format configuration into RenderSession.
And iOS has create a new PR #221 base on this PR.

facebook-github-bot · 2024-12-02T23:03:06Z

@corporateshark has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

corporateshark · 2024-12-03T02:06:27Z

  std::string displayName;
  BackendVersion backendVersion;
  igl::TextureFormat swapchainColorTextureFormat = igl::TextureFormat::BGRA_UNorm8;
-  igl::TextureFormat depthTextureFormat = igl::TextureFormat::Z_UNorm16;


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?

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

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

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.

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.

facebook-github-bot · 2024-12-03T06:33:25Z

@corporateshark has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

corporateshark

This is mixing changes to the mac shell with changes to the iOS shell.

corporateshark · 2024-12-09T04:57:43Z

                             .majorVersion = 3,
                             .minorVersion = 0},
          .swapchainColorTextureFormat = igl::TextureFormat::BGRA_SRGB,
-          .depthTextureFormat = igl::TextureFormat::S8_UInt_Z32_UNorm,


This was a good change. What compilation failure is this fixing?

I have moved the depthTextureFormat field from RenderSessionConfig to RenderSession, so the above code must be removed.

Is it possible to achieve your goal (adding NanoVG) with minimal changes to the current main branch?

I will make the minimal changes as much as possible.

corporateshark · 2024-12-09T04:57:48Z

    currentQuadLayer_ = layer;
  }
+
+  [[nodiscard]] igl::TextureFormat getDepthTextureFormat() const {


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.

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.

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.

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?

This reverts commit 7ba1336.

This reverts commit 74fee9c.

vinsentli · 2024-12-10T01:55:38Z

This is the minimal changes.
iOS is currently appropriate, and no further modifications are needed.

I made modifications to depthTextureFormat in mac/AppDelegate.mm. If you think this is not appropriate, I can revert those changes.

facebook-github-bot · 2024-12-17T22:52:30Z

@corporateshark has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

corporateshark · 2024-12-17T23:02:21Z

                             .majorVersion = 3,
                             .minorVersion = 0},
          .swapchainColorTextureFormat = kColorFramebufferFormat,
+          .depthTextureFormat = igl::TextureFormat::S8_UInt_Z24_UNorm,


This line still stands. We should not enable the stencil buffer unconditionally for all apps.

corporateshark · 2024-12-17T23:04:43Z


 #if IGL_BACKEND_OPENGL
  case igl::BackendFlavor::OpenGL: {
+    const bool enableStencilBuffer = config_.depthTextureFormat == igl::TextureFormat::S8_UInt_Z24_UNorm  ||


If you extract all these changes in shell/mac/ViewController.mm into a separate PR, it will be easier to review and merge because it does not change any existing behaviors.

corporateshark · 2024-12-17T23:27:14Z

Ok, I'm landing the shell/mac/ViewController.mm part of this PR. The AppDelegate.mm part should be reimplemented to keep the existing default depth format.

facebook-github-bot · 2024-12-18T00:43:09Z

@corporateshark merged this pull request in 88143c1.

shell | mac | add stencil buffer

9e3c99d

facebook-github-bot added the cla signed label Nov 26, 2024

vinsentli changed the title ~~shell | mac | add stencil buffer~~ Nov 26, 2024

fix compile error

3ed058e

corporateshark requested changes Nov 26, 2024

View reviewed changes

vinsentli added 2 commits November 27, 2024 18:57

make mac opengl stencil size configurable.

41fab96

Merge branch 'main' into igl_nanovg_part1_mac_add_stencil_buffer

5c7225c

corporateshark reviewed Nov 27, 2024

View reviewed changes

const

d861110

vinsentli requested a review from corporateshark November 27, 2024 11:25

corporateshark requested changes Nov 27, 2024

View reviewed changes

set depth texture format in RenderSession internal.

a80976c

vinsentli requested a review from corporateshark November 28, 2024 03:40

vinsentli mentioned this pull request Dec 2, 2024

iOS : set depth texture format in RenderSession internal. #221

Closed

corporateshark requested changes Dec 3, 2024

View reviewed changes

Fix iOS compile error.

6d42667

vinsentli requested a review from corporateshark December 3, 2024 06:15

corporateshark requested changes Dec 9, 2024

View reviewed changes

vinsentli added 2 commits December 10, 2024 09:45

Revert "set depth texture format in RenderSession internal."

df01ffb

This reverts commit 7ba1336.

Revert "iOS : set depth texture format in RenderSession internal."

4188a2c

This reverts commit 74fee9c.

vinsentli requested a review from corporateshark December 10, 2024 01:55

corporateshark requested changes Dec 17, 2024

View reviewed changes

facebook-github-bot closed this in 88143c1 Dec 18, 2024

facebook-github-bot added the Merged label Dec 18, 2024

vinsentli deleted the igl_nanovg_part1_mac_add_stencil_buffer branch December 18, 2024 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

【igl nanovg part-1】shell | mac | add stencil buffer#216
vinsentli wants to merge 9 commits into
facebook:mainfrom
vinsentli:igl_nanovg_part1_mac_add_stencil_buffer

vinsentli commented Nov 26, 2024

corporateshark left a comment

vinsentli commented Nov 27, 2024

corporateshark Nov 27, 2024

vinsentli Nov 27, 2024

corporateshark left a comment

corporateshark Nov 27, 2024

corporateshark Dec 17, 2024

vinsentli commented Nov 28, 2024

facebook-github-bot commented Dec 2, 2024

corporateshark Dec 3, 2024

vinsentli Dec 3, 2024 •

edited

Loading

corporateshark Dec 3, 2024

vinsentli Dec 3, 2024

facebook-github-bot commented Dec 3, 2024

corporateshark left a comment

corporateshark Dec 9, 2024

vinsentli Dec 9, 2024

corporateshark Dec 9, 2024

vinsentli Dec 10, 2024

corporateshark Dec 9, 2024

vinsentli Dec 9, 2024

corporateshark Dec 9, 2024

vinsentli Dec 9, 2024 •

edited

Loading

vinsentli commented Dec 10, 2024 •

edited

Loading

facebook-github-bot commented Dec 17, 2024

corporateshark Dec 17, 2024

corporateshark Dec 17, 2024

corporateshark commented Dec 17, 2024

facebook-github-bot commented Dec 18, 2024

Labels

3 participants

Uh oh!

Conversation

vinsentli commented Nov 26, 2024

corporateshark left a comment

Choose a reason for hiding this comment

vinsentli commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corporateshark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinsentli commented Nov 28, 2024

facebook-github-bot commented Dec 2, 2024

Choose a reason for hiding this comment

vinsentli Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 3, 2024

corporateshark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinsentli Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

vinsentli commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

facebook-github-bot commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corporateshark commented Dec 17, 2024

facebook-github-bot commented Dec 18, 2024

Labels

3 participants

vinsentli Dec 3, 2024 •

edited

Loading

vinsentli Dec 9, 2024 •

edited

Loading

vinsentli commented Dec 10, 2024 •

edited

Loading