Skip to content

Move render pipelines into separate repos#259

Merged
freezy merged 15 commits into
masterfrom
refactor/rp
Dec 5, 2020
Merged

Move render pipelines into separate repos#259
freezy merged 15 commits into
masterfrom
refactor/rp

Conversation

@freezy
Copy link
Copy Markdown
Owner

@freezy freezy commented Dec 4, 2020

VPE currently supports three render pipelines: The standard built-in renderer, the Universal Render Pipeline and the High Definition Render Pipeline. While built-in becomes less and less relevant, VPE should support both URP and HDRP:

  • URP is going to be important for VR and low-end devices.
  • HDRP will achieve the best visuals on beefier hardware.

Unfortunately, the two RPs are not compatible, i.e. it's not possible to convert a URP scene into HDRP and vice versa, and that's another issue, but it's also impossible to keep both dependencies in the same project (not without weird side effects, anyway).

This PR moves all URP and HDRP related code into separate Unity packages. These are:

Both projects have dependencies to VisualPinball.Engine and VisualPinball.Unity, so they have the full VPE API at their disposal. Within VPE, you can access the current render pipeline anywhere by using RenderPipeline.Current.

New Project Setup

Before this PR, VPE checked whether URP or HDRP was a dependency of the project and converted the materials accordingly. With this PR, VPE checks whether VisualPinball.Unity.Urp or VisualPinball.Unity.Hdrp are in the project. More specifically, it checks whether there are other implementations of IRenderPipeline than the standard (a.k.a built-in) implementation.

That means even though your project is set up with HDRP, VPE won't convert to HDRP, unless VisualPinball.Unity.Hdrp is added as a package as well. It actually can't, because that code is now part of VisualPinball.Unity.Hdrp. So you need to add two packages: The VPE (this) package, and the HDRP package. The same applies for URP.

Patcher Changes

The patcher used to be a dependency of VisualPinball.Unity. That meant it hadn't access to VPE's Unity API, only to the common core API. This PR inverses dependencies. VisualPinball.Unity loads any implementation of IPatcher. Thus, the patcher assembly now has access to VisualPinball.Unity and thus to IRenderPipeline as well, where it can apply render-specific patches.

In fact, it did that before using IRenderPipelinePatcher, which moved into IRenderPipeline as well, as IMaterialAdapter.

Ball Creation

Balls were instantiated via code before and every RP had its own ball creation code. With this PR, ball creation is part of IRenderPipeline, and it returns a GameObject. That allowed us to change the creation approach and use prefabs for ball creation that are much easier to deal with.

TODO

  • Clean up zombie .meta files
  • Add a few more code comments, particularly the interfaces
  • Update documentation

@freezy freezy added editor Editor-related changes for Unity rendering Concerns the HDRP or the URP labels Dec 4, 2020
@freezy freezy requested review from Pandelii and Roland09 December 4, 2020 17:24
@freezy freezy self-assigned this Dec 4, 2020
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 4, 2020

Codecov Report

Merging #259 (c851c3e) into master (66f14f4) will not change coverage.
The diff coverage is 66.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #259   +/-   ##
=======================================
  Coverage   56.79%   56.79%           
=======================================
  Files         159      159           
  Lines        9441     9441           
  Branches      937      937           
=======================================
  Hits         5362     5362           
  Misses       3850     3850           
  Partials      229      229           
Impacted Files Coverage Δ
VisualPinball.Engine/VPT/Mappings/Mappings.cs 77.72% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66f14f4...c851c3e. Read the comment docs.

Copy link
Copy Markdown
Collaborator

@Roland09 Roland09 left a comment

Choose a reason for hiding this comment

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

Looking good. Did some tests with URP and HDRP imports.

I'd change "Built-in" to "Standard" to keep it consistent.

Import works okay, but e. g. HDRP has problems with some materials because of changes from HDRP 8 to HDRP 10. We'll address those after the merge.

Entering play mode takes very long, e. g. Tom & Jerry takes 4 minutes.

Play mode problems:

HDRP: table flickers black
URP: nothing is visible except the lights

Those are problably not related to the refactoring.

@freezy
Copy link
Copy Markdown
Owner Author

freezy commented Dec 5, 2020

@Roland09 Thanks! Where does it still say "Built in" instead of "Standard"?

The URP/HDRP issues seem due to Hybrid Renderer v1, you'll need to use v2.

@freezy freezy merged commit 2c001a1 into master Dec 5, 2020
@freezy freezy deleted the refactor/rp branch December 5, 2020 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

editor Editor-related changes for Unity rendering Concerns the HDRP or the URP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants