Search Unity

[URP 10] Please stop marking everything as Internal and expose more fields

Discussion in 'Universal Render Pipeline' started by Kinetic, Mar 26, 2021.

  1. Kinetic

    Kinetic

    Joined:
    Jul 15, 2012
    Posts:
    26
    The whole idea for SRP is to have a flexible pipeline that can be modified or overriden by us, the developers.

    I'm working in a big project that has some specific rendering needs, and every time I need to modify some aspect of URP, always stumble upon some struct or class that's being marked as internal.

    To point out a simple example, I have to use some nasty reflection to disable or configure SSAO manually because m_RendererFeatures is a private member and SSAO feature is marked as internal, this is completely unacceptable. I remark that this is not the only case, but this was the last straw that prompted me to start this thread.

    Please seriously consider fixing this in your next versions of the package and reevaluate your scope attributes. Yes, I can fork the URP package, but I'd rather prefer to not be forced to do this for some small changes.
     
    funkyCoty, termway, Meceka and 10 others like this.
  2. snacktime

    snacktime

    Joined:
    Apr 15, 2013
    Posts:
    3,356
    Install the Package Development package, which lets you put the package in development mode which puts it into your local packages folder where you can modify it. Then add an InternalsVisibleTo to the AssemblyInfo, or create one if it doesn't exist.
     
    ekakiya and OregonGhost like this.
  3. Kinetic

    Kinetic

    Joined:
    Jul 15, 2012
    Posts:
    26
    So I have to jump over hoops and open a new repo with the package fork to just be able to tweak postprocessing settings in runtime? This is exactly what I don't want to do and hereby my request stands.
     
  4. snacktime

    snacktime

    Joined:
    Apr 15, 2013
    Posts:
    3,356
    You misunderstand. The package development package is an actual package. com.unity.upm.develop.

    With that package, it's a button click to put other packages in dev mode. And adding a couple of lines to the assembly info. It's literally under a minute of work.

    It's what they do for their own packages that do lower level integration with other packages. Unity and actually a good number of large libraries in .Net do this. You have api's you don't want to support as public as that means backwards compatibility and all. But at the same time you want people who have the need to customize something to still have easy access. This is that.
     
  5. BattleAngelAlita

    BattleAngelAlita

    Joined:
    Nov 20, 2016
    Posts:
    400
    I have a feeling that the URP/HDRP developers intentionally do everything to remove any customizability from that renderers
     
  6. BattleAngelAlita

    BattleAngelAlita

    Joined:
    Nov 20, 2016
    Posts:
    400
    This is a ugly kludge. By default they need to provide access to every important internal stuff. Game developers are smart guys, they clearly know what they need to acive and what they need to do for it. Privating all internal things is just way to says "you guys are not smart"
     
    Kinetic likes this.
  7. ElliotB

    ElliotB

    Joined:
    Aug 11, 2013
    Posts:
    284
    Please also note that forking isn't an option for asset store developers
     
  8. Corvwyn

    Corvwyn

    Joined:
    Nov 15, 2013
    Posts:
    114
    That's not what making things private means. I don't know the details in this case, but exposing too much will make it difficult for Unity to make changes in the future without breaking existing games.

    Making good apis/libraries is an art, and you need to strike a good balance. I'm not saying that they shouldn't make more stuff public, just that there might be several reasons that it's private.
     
    buFFalo94 likes this.
  9. BattleAngelAlita

    BattleAngelAlita

    Joined:
    Nov 20, 2016
    Posts:
    400
    I.e. you can't change shadow resolution in runtime without using reflection.
     
    moatdd, Kinetic and Ruslank100 like this.
  10. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,461
    Instead, everyone has to hack into the existing APIs via reflection or asmref's and other tricks, and everything breaks anyway when they change the API. Pretending that locking people out is somehow for our own sake is not some smart design approach- it's just putting extra layers and friction into getting things done.

    I mean, if we follow the logic here, why have any API be anything but internal? Just have every API be internal and require reflection or an asmdef to use it, and then Unity will never ship another breaking change again!

    And they certainly didn't mind designing two render pipelines that literally require rewriting thousands of lines of shader code on every release, so I don't think this is really about code breaking when things change. Unity has been de-prioritizing code stability for a long time now, moving away from things like the script updating system by pushing things into packages and requiring people to update their code instead of having it auto update.

    They have been closing down APIs for a long time now. I had a shader graph node on the store, and had to pull it because they closed the API that made it possible and I got tired of having to hack into an internal API constantly. Closing off APIs so all your stuff is non-extensible by the users is not progress, it's making a closed ecosystem where it's harder to do meaningful work. The fact that you can hack around this is not some smart design choice, but rather than asmdef's and reflection both create massive holes in the encapsulation system.

    I'd much rather have the system be open and have to update code when it changes than the current situation where they pretend you don't need access to basic things, like being able to set a setting from script that you can set in the editor, and then hide it all behind an internal API "for our own good because it might change and that way we don't have to document it".
     
    NotaNaN, Kinetic, Ruslank100 and 3 others like this.
  11. Corvwyn

    Corvwyn

    Joined:
    Nov 15, 2013
    Posts:
    114
    @jbooth No offense or anything. I'm just mentioning what I believe is good practice for designing apis/libraries, and that making code private doesn't mean "you guys are not smart". I'm not saying everything should be internal, just that I don't believe everything should necessarily be public. This is based on writing java applications and libraries for 10 years, so it doesn't necessarily apply to all cases.

    I've seen your work, and if you say this is how things should be, I believe you. A script updating system does sound like a good idea when updating, and would certainly improve this situation.

    Hopefully the situation will improve as URP matures.
     
    Last edited: Mar 29, 2021
  12. jbooth

    jbooth

    Joined:
    Jan 6, 2014
    Posts:
    5,461
    Oh certainly good API's are always a challenge and not everything should be public- but Unity has been closing off APIs that should be public in what feels like a defensive manner, rather than a constructive one. For me, Unity is all about scriptability- and things which aren't scriptable always feel odd to me.

    Unity has a script updating system already- it's pretty amazing, and allows developers to write for old versions of Unity and have most things automatically fixed when they upgrade. I'm sure it's a pain to maintain, as every change to the existing APIs requires them to write an auto-update function, but it's got to be a tiny fraction of the work of 5 million users upgrading their projects individually. However, as they moved things into packages they basically stopped doing this for package code. And this is what we've seen a lot lately- Unity changing things to save themselves some trouble, at the cost of thousands of projects having to do the same work on upgrade. Want to switch render pipelines? Rewrite every shader - rather than having a proper abstraction layer between them.

    I always thought the point of licensing engine code was to have the engine company save you time- not create new ways to spend your time.
     
    Corvwyn likes this.
  13. snacktime

    snacktime

    Joined:
    Apr 15, 2013
    Posts:
    3,356
    Ya the whole black boxing stuff is annoying. Which is different then simply having a line where they expect you to use the source for more advanced level customization. And having different policies for public vs (logically) internal api's.

    UE4 for example has a lot that is internal in that it requires using the source to customize. But it's intended that you will do so. You are expected to deal with the internal churn api's might go through between versions. Yet at the same time it's designed to be modified with a lot of hooks towards that end.

    Unity acted for a time like they were going to try and open more up. But they walked it back and actually ended up more closed then where they started. Rendering has definitely been the worst case, some other areas are not nearly as bad.
     
    Kinetic likes this.
  14. Kinetic

    Kinetic

    Joined:
    Jul 15, 2012
    Posts:
    26
    The irony here is that the whole point of the SRP package is to make extensible render pipelines. And then they release URP with everything internal?

    Look at this, this is the implementation I had to go through to add SSAO settings in decentraland explorer:
    https://github.com/decentraland/exp...cificControllers/SSAOControlController.cs#L25

    I think this problem can't be more clearly stated after looking at that file that just attempts to toggle renderer features. Nothing fancy.
     
    bluescrn likes this.
  15. Kinetic

    Kinetic

    Joined:
    Jul 15, 2012
    Posts:
    26
    Another annoying case with shadergraph that's related with this policy:

    With the new master stack implementation, now the Lit, Unlit, etc, modes are completely hardcoded into the shadergraph implementation (and of course with no public access). So this is even a step backwards from the custom master node and a step forward towards closed libraries. I needed an Unlit with SSAO support. To achieve this simple thing I had to use the Lit master stack, extract the compiled code of the graph and modify it to inject a custom forward pass. There was no way to add a new mode or extend the stack in a way that worked.

    Here's the hack if anyone's interested:
    https://github.com/decentraland/exp...client/Assets/Rendering/Shaders/Toon/Compiled
     
    Last edited: Apr 16, 2021
    PutridEx likes this.
  16. Oxeren

    Oxeren

    Joined:
    Aug 14, 2013
    Posts:
    121
    Yep, I've encountered these kind of issues as well. Sometimes even pretty trivial stuff is internal.
    My guess is that Unity now has some kind of internal policies about public APIs, maybe there are some kind of development overhead for making stuff public, like adding some additional tests, or maybe some managerial procedures or whatever. Maybe these are even good practices, but yeah, in the end extending SRPs is often more difficult than it needs to be.
     
  17. Thygrrr

    Thygrrr

    Joined:
    Sep 23, 2013
    Posts:
    700
    Rejoice! Unity Technologies resolved this by making URP 11.0 no longer able to be put into development mode; really classy, I must say. /s

    I'm trying to patch the Lit shader and now I can't even do that without seriously screwing around in Unity's guts... or duplicating and constantly maintaining a couple dozen HLSL includes, where I basically just need to add three uniform half3's to Input.hlsl (and do something with them in the GI lookups).
     
    termway likes this.
  18. bluescrn

    bluescrn

    Joined:
    Feb 25, 2013
    Posts:
    642
    Just ran into this exact same thing - just wanted to decrease the SSAO radius from it's usual default while using a specific camera.

    It should be obvious that this sort of thing should need to be scriptable by default, not private+internal with no accessors :(
     
  19. termway

    termway

    Joined:
    Jul 5, 2012
    Posts:
    80
    Two years later and it is still a mess (Unity 2022 LTS & URP 14).

    I can also confirm that you cannot put URP in development mode anymore.

    In my own use case, I wanted to extend the new FullScreenPassRendererFeature, but I cannot do that because certain elements have internal visibility for some reason.
     
  20. AljoshaD

    AljoshaD

    Unity Technologies

    Joined:
    May 27, 2019
    Posts:
    229
    That's definitely true. The fact of the matter is that backwards compatibility or upgradability or changing a public API has quite a high cost for Unity, easily dwarfing the work on the feature or API itself. Sometimes a changed API can auto upgrade and we have a great tool for that but in many cases that doesn't work. So developers are very mindful - to the point that they might play it too safe - of making something public. We know how painful it can be to need to upgrade an API so this is always top of mind.

    However, we really focused on the extensibility for URP in Unity 2023. Also this is difficult to change for the better, since change means work for our users and asset providers, but we have accomplished opening up the rendering and allow you to write extensions using the same API as our internal developers. We call this the RenderGraph project but the API improvements are much wider than strictly RenderGraph. You can read more about that here. Also in this project probably 1/3 of our time if spend on facilitating upgrading. And the end result, ie, performance benefits of RG, are limited in 2023 because we ensure a fallback path (called Compatibility mode). It's a difficult balancing act that we are very mindful of.
     
  21. funkyCoty

    funkyCoty

    Joined:
    May 22, 2018
    Posts:
    726
    You wrote all of this to tell us changing "internal" to "public" on shadow quality, ssao, and others is "difficult to change" omg
     
    goncalo-vasconcelos likes this.
  22. ElliotB

    ElliotB

    Joined:
    Aug 11, 2013
    Posts:
    284
    The rendergraph API is great but there's still a large amount of URP's functionality that is hidden behind internal. In addition to those above, SG ShaderPass structs and methods are all internal, which prevents creating new URP subtargets for SG that are based on those shipped with URP. So if you want to ship a custom SG subtarget that overrides just a single aspect (eg lighting), you then need to manually migrate between changing keywords on each version, but _also_ embed URP to expose internals. At least if the API was public we wouldn't have to embed.

    I appreciate the explanation of some of Unity's internal considerations and constraints. I'd counter that with URP we are in a position where the public API changes so often* that these are only adding the overheads and not the benefits.

    *to be clear, I mean the API and its behavior. If the API is the same, but the behavior changed between minor versions so we now need to #define and blit a bunch of separate targets then it might aswell be considered a change because it requires different code to execute correctly on different platforms.

    Do you think the URP team could consider opening the API up?

    Cheers
     
    goncalo-vasconcelos likes this.