-
Notifications
You must be signed in to change notification settings - Fork 47
Fix macOS runner lightmap and metal issues #1264
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @a-maurice, 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!
Summary of Changes
Hello team,
Gemini here, providing a summary of this pull request by @a-maurice. The primary goal of this PR is to address recent timeouts observed on macOS GitHub Actions runners when building the test applications. According to the description and linked issue, these timeouts appear to be related to Unity's lightmapping process, specifically an inability to fall back to the CPU lightmapper in the runner environment. Additionally, the PR aims to ensure consistent support for the Metal graphics API across the testapps.
The changes involve modifying Unity scene files (.unity
) and project settings files (ProjectSettings.asset
) within several testapp directories. The modifications adjust lightmap settings within scenes and enable Metal editor support in the project settings. These changes are intended to configure the test environments in a way that avoids the lightmapping issues and ensures Metal compatibility on the macOS runners, thereby resolving the build timeouts.
Highlights
- Fix macOS GHA Timeouts: The core purpose of this PR is to fix build timeouts occurring on macOS GitHub Actions runners, which are attributed to issues with Unity's lightmapping process in that environment.
- Enable Metal Support: The pull request ensures that Metal editor support is consistently enabled across the various test applications by modifying their
ProjectSettings.asset
files. - Adjust Lightmap Settings: Changes are made to lightmap settings within several test scene files (
.unity
), including enabling baked/realtime lightmaps and changing the GI workflow mode, likely to mitigate the lightmapping issues causing timeouts.
Changelog
Click here to see the changelog
- analytics/testapp/Assets/Firebase/Sample/Analytics/MainScene.unity
- Enabled baked lightmaps (
m_EnableBakedLightmaps
changed from 0 to 1). - Enabled realtime lightmaps (
m_EnableRealtimeLightmaps
changed from 0 to 1).
- Enabled baked lightmaps (
- analytics/testapp/Assets/Firebase/Sample/Analytics/MainSceneAutomated.unity
- Changed GI Workflow Mode (
m_GIWorkflowMode
) from 0 (NoGI) to 1 (Baked).
- Changed GI Workflow Mode (
- analytics/testapp/ProjectSettings/ProjectSettings.asset
- Enabled Metal editor support (
metalEditorSupport
changed from 0 to 1).
- Enabled Metal editor support (
- auth/testapp/ProjectSettings/ProjectSettings.asset
- Enabled Metal editor support (
metalEditorSupport
changed from 0 to 1).
- Enabled Metal editor support (
- crashlytics/testapp/Assets/Firebase/Sample/Crashlytics/MainSceneAutomated.unity
- Changed GI Workflow Mode (
m_GIWorkflowMode
) from 0 (NoGI) to 1 (Baked).
- Changed GI Workflow Mode (
- crashlytics/testapp/ProjectSettings/ProjectSettings.asset
- Enabled Metal editor support (
metalEditorSupport
changed from 0 to 1).
- Enabled Metal editor support (
- database/testapp/ProjectSettings/ProjectSettings.asset
- Enabled Metal editor support (
metalEditorSupport
changed from 0 to 1).
- Enabled Metal editor support (
- dynamic_links/testapp/ProjectSettings/ProjectSettings.asset
- Enabled Metal editor support (
metalEditorSupport
changed from 0 to 1).
- Enabled Metal editor support (
- functions/testapp/ProjectSettings/ProjectSettings.asset
- Enabled Metal editor support (
metalEditorSupport
changed from 0 to 1).
- Enabled Metal editor support (
- scripts/gha/integration_testing/MenuScene/Menu.unity
- Changed GI Workflow Mode (
m_GIWorkflowMode
) from 0 (NoGI) to 1 (Baked).
- Changed GI Workflow Mode (
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 is currently in preview and 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 to provide feedback.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to resolve issues with macOS GHA runners timing out during testapp builds, specifically related to lightmap baking and Metal support. The changes to enable Metal editor support across the various testapps (ProjectSettings.asset
files) are a positive step and align well with the goal of supporting Metal.
However, there are a couple of significant concerns regarding the lightmap setting modifications that need clarification to ensure the PR achieves its primary objective of preventing lightmap-related timeouts:
- One scene file (
analytics/.../MainScene.unity
) has changes that appear to enable baked and realtime lightmaps, which seems to contradict the stated goal of "consistently not bake the lightmaps." - Other scene files are switching from a Baked GI workflow to Realtime GI. While this avoids pre-baking, it's important to confirm that Realtime GI itself doesn't trigger the lightmapper issues on the virtualized macOS runners.
Summary of Findings
- Contradictory Lightmap Enablement: In
analytics/testapp/Assets/Firebase/Sample/Analytics/MainScene.unity
, settingsm_EnableBakedLightmaps
andm_EnableRealtimeLightmaps
were changed from0
to1
. This appears to enable lightmap features, which contradicts the PR's stated goal to "consistently not bake the lightmaps" and could prevent the fix for GHA timeouts. This is a critical concern. - Effectiveness of Realtime GI on GHA Runners: Several scenes (
MainSceneAutomated.unity
for analytics and crashlytics,Menu.unity
) were switched fromm_GIWorkflowMode: 0
(Baked Indirect) to1
(Realtime GI). While this avoids pre-baking, it's important to confirm that Realtime GI itself doesn't trigger the lightmapper issues on virtual machines, as indicated in the linked Unity issue. For test applications, completely disabling GI might be a more robust solution if specific lighting fidelity is not a requirement. - Metal Support: The changes enabling
metalEditorSupport: 1
in variousProjectSettings.asset
files are positive and directly address the goal of supporting Metal on macOS runners.
Merge Readiness
The pull request makes good progress in enabling Metal support for the testapps. However, due to the critical concern regarding the lightmap settings in analytics/testapp/Assets/Firebase/Sample/Analytics/MainScene.unity
and the high-severity questions about the general strategy for disabling lightmap baking (Realtime GI vs. fully disabled GI), I recommend that these issues be thoroughly addressed before merging. As a reviewer, I am not authorized to approve pull requests; please ensure these points are resolved and further review is conducted if necessary.
m_EnableBakedLightmaps: 1 | ||
m_EnableRealtimeLightmaps: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description states the goal is to "consistently not bake the lightmaps" to avoid GHA runner timeouts. However, these changes from 0
to 1
for m_EnableBakedLightmaps
and m_EnableRealtimeLightmaps
seem to enable these lightmap features.
Could you clarify the reasoning behind enabling these? If the intent is to prevent any lightmap baking or processing that could cause timeouts (as suggested by the linked Unity issue https://issuetracker.unity3d.com/issues/unable-to-fall-back-to-cpu-lightmapper-dot-errors-in-the-console-when-opening-a-project-on-a-virtual-machine
), shouldn't these values be 0
to explicitly disable baked and realtime lightmaps for this scene?
m_EnableBakedLightmaps: 0
m_EnableRealtimeLightmaps: 0
@@ -42,7 +42,7 @@ RenderSettings: | |||
LightmapSettings: | |||
m_ObjectHideFlags: 0 | |||
serializedVersion: 7 | |||
m_GIWorkflowMode: 0 | |||
m_GIWorkflowMode: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing m_GIWorkflowMode
from 0
(Baked Indirect) to 1
(Realtime GI) correctly avoids the pre-baking of lightmaps, aligning with the PR's goal.
However, Realtime GI still involves significant lighting computations and might interact with the lightmapping system on the macOS GHA runners. Given the linked Unity issue (https://issuetracker.unity3d.com/issues/unable-to-fall-back-to-cpu-lightmapper-dot-errors-in-the-console-when-opening-a-project-on-a-virtual-machine
) points to problems with the lightmapper itself on VMs, is switching to Realtime GI sufficient to prevent timeouts?
For testapps, which often don't require complex lighting, would it be more robust to consider completely disabling Global Illumination features (e.g., by ensuring m_EnableBakedLightmaps: 0
, m_EnableRealtimeLightmaps: 0
, and potentially adjusting m_GIWorkflowMode
accordingly, perhaps to 0
if all GI is off) to minimize any interaction with the lightmapper?
@@ -42,7 +42,7 @@ RenderSettings: | |||
LightmapSettings: | |||
m_ObjectHideFlags: 0 | |||
serializedVersion: 7 | |||
m_GIWorkflowMode: 0 | |||
m_GIWorkflowMode: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment on analytics/testapp/Assets/Firebase/Sample/Analytics/MainSceneAutomated.unity
:
Changing m_GIWorkflowMode
from 0
(Baked Indirect) to 1
(Realtime GI) avoids pre-baking. However, could Realtime GI still trigger issues on the macOS GHA runners due to the underlying lightmapper problems on VMs (as per the linked Unity issue)?
If these testapps don't strictly need dynamic global illumination, would completely disabling GI (e.g., m_EnableBakedLightmaps: 0
, m_EnableRealtimeLightmaps: 0
) be a safer approach to ensure stability on the runners?
@@ -43,7 +43,7 @@ RenderSettings: | |||
LightmapSettings: | |||
m_ObjectHideFlags: 0 | |||
serializedVersion: 9 | |||
m_GIWorkflowMode: 0 | |||
m_GIWorkflowMode: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to comments on other scenes changing m_GIWorkflowMode
:
This change to m_GIWorkflowMode: 1
(Realtime GI) avoids pre-baking lightmaps. However, considering the context of macOS GHA runner timeouts and the linked Unity issue regarding lightmapper problems on VMs, is Realtime GI guaranteed to be stable?
If this menu scene doesn't require dynamic global illumination, would it be more robust to ensure all GI features are disabled (e.g., m_EnableBakedLightmaps: 0
, m_EnableRealtimeLightmaps: 0
) to prevent any lightmapper invocation?
Description
After updating, the macOS GHA runners have been timing out when building the testapps, related to: https://issuetracker.unity3d.com/issues/unable-to-fall-back-to-cpu-lightmapper-dot-errors-in-the-console-when-opening-a-project-on-a-virtual-machine
Change the testapps to consistently not bake the lightmaps and support metal. Some of them already were.
Testing
https://github.com/firebase/firebase-unity-sdk/actions/runs/15483427151
Type of Change
Place an
x
the applicable box: