Feature/urp blur #135
Feature/urp blur #135
Conversation
…completed in the unit test
…to the JSON Schema draft‑2020‑12
…o handle if enum is included as a string field
…json which is aligned with the latest 2020 schema specs (and with that needed e.g. when using json schemas in the OpenAi APIs)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @cs-util, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the codebase by integrating a custom blur shader for Unity's Universal Render Pipeline (URP), refining OpenAI API interactions with stricter JSON schema validation, and automating .NET testing through a new GitHub Actions workflow. It also includes various quality-of-life improvements across the C# utility libraries.
Highlights
- OpenAI API Refinements: Updated API calls to use max_completion_tokens instead of max_tokens and introduced robust validation for JSON schema responses, ensuring strict adherence to OpenAI's requirements.
- Unity URP Blur Shader: Implemented a new custom blur shader and associated assets for Unity's Universal Render Pipeline, enabling advanced visual effects for UI elements.
- Automated .NET Testing: Established a new GitHub Actions workflow to automatically build, run, and report .NET tests, including safe caching and exclusion of integration tests for efficient CI.
- Improved JSON Schema Generation: Enhanced the JSON schema generation logic to better handle enum types, custom JSON property names, and array item definitions using anyOf.
- Enum Validation Enhancement: Modified enum extension methods to prevent unnecessary warnings for system-defined enums during power-of-two checks.
- Asynchronous Task Management: Added new methods to ExpensiveRegularAsyncTaskInBackground and TemplatesIO to allow waiting for all background tasks to complete, improving testability and control.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Summary of Changes
Hello @cs-util, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the codebase by integrating new features and improving existing functionalities across three main areas: OpenAI API interactions, Unity UI rendering, and CI/CD processes. It introduces a custom blur shader for Unity's Universal Render Pipeline, refines OpenAI API request handling with stricter JSON schema validation and updated token management, and establishes a new GitHub Actions workflow for automated .NET test execution and reporting.
Highlights
- OpenAI API Refinements: Updated "max_tokens" to "max_completion_tokens" for alignment with OpenAI's latest API, removed manual token calculation from constructors, and implemented stricter validation for JSON schema response formats to ensure compatibility and robustness. A new convenience method simplifies setting JSON schema response formats.
- Unity URP Blur Shader: Added a custom blur shader and material for Unity's Universal Render Pipeline, enabling advanced blur effects for UI elements and improving visual fidelity.
- Automated .NET Testing: Introduced a new GitHub Actions workflow to automate building, running, and reporting .NET tests, including safe caching and exclusion of integration tests for faster feedback, enhancing CI/CD efficiency.
- Improved Enum Validation: Modified enum validation logic to exclude system enums from power-of-two checks, reducing irrelevant warnings for types outside developer control.
- JSON Schema Handling Improvements: Updated "JsonModelExtensions.cs" and "JsonSchema.cs" to correctly handle "anyOf" in array items, improving compatibility with complex JSON schemas. "ModelToJsonSchema.cs" now includes "StringEnumConverter" and better handles "JsonPropertyAttribute" for field names.
- Asynchronous Task Management: Added "WaitTillDone" methods to "ExpensiveRegularAsyncTaskInBackground.cs" and "TemplatesIO.cs" to allow for explicit waiting until all background tasks are completed, improving control over asynchronous operations.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new blur shader for URP, enhances the OpenAI API integration with stricter validation and new features, and adds a new CI workflow for .NET tests. My review focuses on a potential performance issue in the new blur shader and a correctness issue in the CI workflow. The OpenAI and other C# code changes look solid and improve the codebase.
| report: | ||
| needs: build-test | ||
| if: github.event_name != 'pull_request' | ||
| || github.event.pull_request.head.repo.full_name == github.repository | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| checks: write | ||
| pull-requests: write | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Upload and annotate test results | ||
| uses: dorny/test-reporter@v1 | ||
| with: | ||
| name: Test Results | ||
| path: '**/*.trx' | ||
| reporter: dotnet-trx | ||
| fail-on-error: 'true' |
There was a problem hiding this comment.
The report job runs on a separate runner from build-test and won't have access to the test results file. The build-test job needs to upload the .trx file as an artifact, and this report job needs to download it before trying to use it. Without this, the dorny/test-reporter action will fail as it won't find the test results.
Here's how you can fix it:
-
In the
build-testjob, after running tests, upload the results:- name: Upload test results if: always() # Run this even if tests fail uses: actions/upload-artifact@v4 with: name: test-results path: '**/*.trx'
-
In this
reportjob, before uploading results, download them:- name: Download test results uses: actions/download-artifact@v4 with: name: test-results - name: Upload and annotate test results uses: dorny/test-reporter@v1 with: name: Test Results path: '**/*.trx' # This will now find the downloaded file reporter: dotnet-trx fail-on-error: 'true'
| for (int x = -_Samples / 2; x <= _Samples / 2; x++) | ||
| { | ||
| for (int y = -_Samples / 2; y <= _Samples / 2; y++) | ||
| { | ||
| float2 offset = float2(x, y) * _Size * _CameraOpaqueTexture_TexelSize.xy; | ||
| float distance = length(offset); | ||
|
|
||
| // Gaussian weight based on distance | ||
| float weight = exp(-distance * distance * 0.5); | ||
|
|
||
| float2 sampleUV = uvScreen + offset; | ||
|
|
||
| // Sample the opaque texture (which should contain all rendered content including world space UI) | ||
| half3 sample = tex2D(_CameraOpaqueTexture, sampleUV).rgb; | ||
|
|
||
| result += sample * weight; | ||
| totalWeight += weight; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current blur implementation uses a nested loop, which results in (_Samples + 1) * (_Samples + 1) texture samples per fragment. With the default _Samples value of 16, this is 289 samples, and it can go up to 33*33 = 1089 samples. This is computationally very expensive and will likely cause significant performance issues, especially on mobile devices or lower-end GPUs.
For a much more performant Gaussian blur, consider implementing a two-pass approach. This involves:
- A first pass that blurs the image horizontally.
- A second pass that blurs the result of the first pass vertically.
This reduces the number of texture samples from O(N²) to O(2N), providing a huge performance boost for a visually similar result. This would require changes to how the shader is used (e.g., using a CommandBuffer or a second camera with a render texture in Unity), but it's the standard and recommended way to implement an efficient blur.
There was a problem hiding this comment.
Code Review
This pull request introduces a range of valuable features, including OpenAI API enhancements, a new URP blur shader, and a CI workflow for .NET tests. The changes to the OpenAI client and JSON schema handling are well-implemented and improve robustness. My review focuses on the new CI workflow and the blur shader, where I've identified a couple of issues. There's a critical issue in the GitHub Actions workflow that will prevent test reporting from working correctly, and a high-severity performance issue in the new blur shader. Addressing these will significantly improve the quality of this contribution.
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Upload and annotate test results | ||
| uses: dorny/test-reporter@v1 | ||
| with: | ||
| name: Test Results | ||
| path: '**/*.trx' | ||
| reporter: dotnet-trx | ||
| fail-on-error: 'true' |
There was a problem hiding this comment.
The report job runs in a separate environment from build-test and won't have access to the test-results.trx file. The dorny/test-reporter action will fail because it cannot find the test results file.
To fix this, you need to upload the test results as an artifact in the build-test job and then download that artifact in this report job before running the reporter.
First, add this step to the end of your build-test job:
- name: Upload test results
uses: actions/upload-artifact@v4
with:
name: test-results
path: '**/*.trx'
retention-days: 1Then, you can replace the steps in this report job with the suggested code, which downloads the artifact. The Checkout code step is likely no longer necessary.
steps:
- name: Download test results
uses: actions/download-artifact@v4
with:
name: test-results
- name: Upload and annotate test results
uses: dorny/test-reporter@v1
with:
name: Test Results
path: '**/*.trx'
reporter: dotnet-trx
fail-on-error: 'true'
| for (int x = -_Samples / 2; x <= _Samples / 2; x++) | ||
| { | ||
| for (int y = -_Samples / 2; y <= _Samples / 2; y++) | ||
| { | ||
| float2 offset = float2(x, y) * _Size * _CameraOpaqueTexture_TexelSize.xy; | ||
| float distance = length(offset); | ||
|
|
||
| // Gaussian weight based on distance | ||
| float weight = exp(-distance * distance * 0.5); | ||
|
|
||
| float2 sampleUV = uvScreen + offset; | ||
|
|
||
| // Sample the opaque texture (which should contain all rendered content including world space UI) | ||
| half3 sample = tex2D(_CameraOpaqueTexture, sampleUV).rgb; | ||
|
|
||
| result += sample * weight; | ||
| totalWeight += weight; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current blur implementation uses a single pass with a nested loop, which has a computational complexity of O(N²), where N is proportional to the _Samples property. This can be very performance-intensive, especially on mobile devices or with a high sample count and blur radius. For example, with _Samples at its maximum value of 32, this loop runs over 1000 times per fragment.
A much more performant approach is to use a two-pass blur. This involves:
- A first pass that blurs the image horizontally.
- A second pass that takes the result of the first and blurs it vertically.
This reduces the complexity to O(2N), providing a significant performance boost for a visually similar result. Consider refactoring this to a two-pass shader for better performance.
| sampler2D _CameraColorTexture; | ||
| float4 _CameraColorTexture_TexelSize; |
There was a problem hiding this comment.
These uniforms are declared but not used within the shader. They should be removed to keep the code clean and avoid confusion.
1 participant
This pull request introduces several improvements and new features across the codebase, focusing primarily on OpenAI API integration, Unity UI enhancements, and CI workflow automation. The most significant changes include stricter validation and configuration for OpenAI chat requests, the addition of a custom blur shader for Unity's Universal Render Pipeline (URP), and a new GitHub Actions workflow for running and reporting .NET tests.
OpenAI API improvements:
max_tokensproperty withmax_completion_tokensinOpenAi.csto align with the latest OpenAI API specification and clarified its usage in both request classes. The constructors were updated to remove manual token calculation logic. [1] [2] [3]additionalPropertiesis set to true, if the schema name does not match the required pattern, or if the schema type is not"object". This ensures compatibility with OpenAI's strict JSON schema requirements. [1] [2]Unity UI and rendering enhancements:
BlurUrp.shader) and associated material (BlurUrp.mat) for the Universal Render Pipeline, enabling advanced blur effects for UI elements. Included all necessary Unity asset metadata files for integration. [1] [2] [3] [4] [5] [6]Continuous Integration and Testing:
dotnet-tests.yml) for building, running, and reporting .NET tests, with safe caching and exclusion of integration tests for faster feedback. Automated test result reporting is included for non-forked pull requests.Miscellaneous improvements:
JsonModelExtensions.csto use the correct schema source (anyOf) when creating new list entries and child views, improving compatibility with complex JSON schemas. [1] [2]usingstatements inOpenAi.csfor regex and model support.This change is