Skip to content

APV Baking States#6415

Merged
FrancescoC-unity merged 37 commits into
masterfrom
apv/baking-states
Jan 14, 2022
Merged

APV Baking States#6415
FrancescoC-unity merged 37 commits into
masterfrom
apv/baking-states

Conversation

@adrien-de-tocqueville
Copy link
Copy Markdown
Contributor

@adrien-de-tocqueville adrien-de-tocqueville commented Nov 29, 2021

Purpose of this PR

This PR adds baking states for APV. It allows to bake lighting in different scene configurations that can then be used at runtime when needed.

First here's as gif showing how it works. Note that there's a script changing the lights when the state is changed
apv-states

New UI: Total number of states is fixed to 16, and the names are shared between sets, but the content can be baked separately, and some sets can choose to use only some of the states
image

Added new api in ProbeReferenceVolume

  • bakingState to set/get the current active state
  • onBakingStateChanged, a delegate so that scripts can register to state change

I also changed a bit the logic when selecting "Generate Lighting > Bake the set", which now doesn't require saving and reloading the scenes if the scenes in the set are already currently loaded.
Also "clear baked data" now delete the asset on disk

Notes:

  • To detect which bake is out of date, it relies on a hash compute during baking. It will need to be updated if we add more paraemters in the future

Testing status

Tested baking multiple states, also tested in a setup with multiple sets

tlaedre and others added 5 commits November 2, 2021 20:29
…ialization slowdown (affects domain reloads). Store shader data pre-swizzled (priming a future loading PR).
…ebug display checked. Hide PV debug options when they are not available (and the entire panel when none are)
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 29, 2021

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/jobDefinition/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@github-actions github-actions Bot added the SRP label Nov 29, 2021
Comment thread com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.cs Outdated
Comment thread com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.cs Outdated
@FrancescoC-unity
Copy link
Copy Markdown
Contributor

There are a bunch of things we need to discuss about this actually before I go on with the review, give me a ping when you around for a call

Comment thread com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.cs Outdated
@FrancescoC-unity
Copy link
Copy Markdown
Contributor

Also adding @alelievr as there are touches to the baking window.

Comment thread com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeVolumeAsset.cs Outdated
FrancescoC-unity and others added 3 commits December 16, 2021 14:35
# Conflicts:
#	com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs
#	com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.cs
Copy link
Copy Markdown
Contributor

@TomasKiniulis TomasKiniulis left a comment

Choose a reason for hiding this comment

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

Continuing on Motiejus testing. I seem to be running into lighting issues when switching between two states after I use same states in multiple Baking Sets. Probe volumes themselves remain unchanged but the lighting is way off. Maybe a potential bug to lighting team or not pr related, but the states feature reveal this.

Running into some UX issues where I seem to be not getting "Out Of Date" state usually when having multiple sets. Also loading scenes from another set does not load probes from state, but the state is highlighted.

I also find it confusing switching states, but having to manually switch through volume lighting setups or different lighting scenes, would be good to have some sort of functionality directly in the ui to disable/enable specific volumes load/unload scenes, I guess that's something planned for the future?

Listed my findings in test doc: https://confluence.unity3d.com/display/HDRP/APV+Baking+State could you have a look? And let me know if more info is needed.

Fix state status, fix state loading when changing scene or domain reload
@iM0ve iM0ve removed their request for review January 6, 2022 10:54
Copy link
Copy Markdown
Contributor

@TomasKiniulis TomasKiniulis left a comment

Choose a reason for hiding this comment

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

Looks good! Issues listed in test doc are addressed for this PR. Status seems to be properly shown after bake and clearing or not saving the scene. Baking States table is hidden when no Scene is assigned.

@adrien-de-tocqueville
Copy link
Copy Markdown
Contributor Author

UX is back to this point, where number of state is not fixed, and states can be different between two baking sets.
(Note however that they are identified by names, so if you switch between sets that have states with the same name, it will automatically be loaded)
image

@adrien-de-tocqueville
Copy link
Copy Markdown
Contributor Author

@FrancescoC-unity right now since the state name is appended to the baked data file name on disk, existing project will need to rebake. I didn't do a conversion step but i could do it if you think it's important

@FrancescoC-unity
Copy link
Copy Markdown
Contributor

@FrancescoC-unity right now since the state name is appended to the baked data file name on disk, existing project will need to rebake. I didn't do a conversion step but i could do it if you think it's important

I think it's fine; we are still in experimental so it's fine breaking. Only thing is remember to ping the slack channel when this lands.

Copy link
Copy Markdown
Contributor

@TomasKiniulis TomasKiniulis left a comment

Choose a reason for hiding this comment

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

Gave another look after the changes. Looks good! Haven't ran into any PR specific issues

Copy link
Copy Markdown
Contributor

@iM0ve iM0ve left a comment

Choose a reason for hiding this comment

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

Tomas tested the PR. Couldnt remove myself, approving.

@FrancescoC-unity FrancescoC-unity merged commit 5a9dd79 into master Jan 14, 2022
@FrancescoC-unity FrancescoC-unity deleted the apv/baking-states branch January 14, 2022 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants