Skip to content
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

Add immediate mode debug rendering subsystem for primitives. #517

Closed
wants to merge 7 commits into from

Conversation

profan
Copy link
Contributor

@profan profan commented Sep 21, 2019

PR Details

Adds a new assembly, Stride.DebugRendering, as well as a new render feature to Stride for rendering debug primitives and integrates it into the core.

It is also made accessible in ScriptComponent like DebugText is.

A functioning version of this exact PR can also be found here: XenkoDebugRendering

edit: .. though the old one is for Xenko now, so a bit out of date, but it's the same code really.

Description

This PR adds a RootRenderFeature which is the actual renderer, as well as a GameSystem which handles the buffering of drawing commands and generally serves as the user's interface towards it.

All rendering occurs in batches as much as possible, minimizing the amount of drawcalls and separate buffers necessary, allowing the user to render a large amount of debug primitives per frame without any large impact.

Overview of the current API:
image

Problems (to consider before merging)

  • Currently the shaders necessary for this feature to function are not automatically included (maybe gracefully handling these shaders not being included might also be a good idea either way rather than just bubbling the exception like right now?), adding a reference to Stride.DebugRendering to the project manually fixes the problem, but it would be best to avoid this being necessary. (now resolved)
  • The VisibilityGroup used when adding the render objects (inside the DebugRenderSystem) is fetched using RenderContext.GetShared(Services) but I don't quite yet understand the lifecycle of the VisibilityGroup stuff if/when it may change and what what would be the best way to handle this, so need you to look at this @xen2.
    Fetching it like the other in-game systems do is possible but would make it necessary to introduce a hard dependency on Xenko.Engine and then there's a circular dependency, so there needs to be a way to solve this without introducing a circular dependency if it should live in its own assembly.
  • Currently a MaxPrimitives and MaxPrimitivesWithLifetime are both set to 1024 to cut off rendering any more primitives past that point (but can be set to an arbitrary value by the user), this is primarily to avoid a situation where the user consumes all of their memory by rendering a ton of debug primitives with a duration > 0 (so they'd persist between frames but the user might be adding more every frame, eating all their memory), maybe consider if this is only necessary for primitives with lifetime as that one is easier to mess up?
  • Thread safety of pushing messages to the DebugRenderSystem, should it be considered? (will be thread unsafe by default, as other game systems are generally, hopefully fine)

Related Issue

Related to issue #28.

Motivation and Context

It's not uncommon to run into a need to visualize things such as raycasts, shape sweeps or velocity vectors when writing game code in a 3D context.

if (Input.IsMouseButtonPressed(Xenko.Input.MouseButton.Left))
{
    var clickPos = Input.MousePosition;
    var result = Utils.ScreenPositionToWorldPositionRaycast(clickPos, CurrentCamera, this.GetSimulation());
    if (result.Succeeded)
    {
        var cameraWorldPos = CurrentCamera.Entity.Transform.WorldMatrix.TranslationVector;
        var cameraWorldUp = CurrentCamera.Entity.Transform.WorldMatrix.Up;
        var cameraWorldNormal = Vector3.Normalize(result.Point - cameraWorldPos);
        DebugDraw.DrawLine(cameraWorldPos + (cameraWorldNormal * -2.0f) + (cameraWorldUp * (-0.125f / 4.0f)), result.Point, color: Color.HotPink, duration: 5.0f);
        DebugDraw.DrawArrow(result.Point, result.Point + result.Normal, color: Color.HotPink, duration: 5.0f);
        DebugDraw.DrawArrow(result.Point, result.Point + Vector3.Reflect(result.Point - cameraWorldPos, result.Normal), color: Color.LimeGreen, duration: 5.0f);
    }
}

image

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@profan profan changed the title implement immediate mode debug rendering subsystem for primitives. Add immediate mode debug rendering subsystem for primitives. Sep 21, 2019
@Aggror
Copy link
Member

Aggror commented Sep 21, 2019

oohhh sweet addition. Thanks for that.

@profan profan force-pushed the feature/debugrender branch 4 times, most recently from a532d4a to d774e86 Compare September 24, 2019 18:55
@profan
Copy link
Contributor Author

profan commented Sep 24, 2019

Also added rendering half spheres at @Eideren's request.
image

image

Might add a special render mode later for combined wireframe + solid in one render message + then some kind of automatic colour modulation to make the wireframe actually visible, not now though. 👀

@profan profan force-pushed the feature/debugrender branch 5 times, most recently from c21fc67 to 2be503c Compare September 24, 2019 20:47
@profan
Copy link
Contributor Author

profan commented Sep 30, 2019

Potentially need to look into if StructuredBuffer actually works at all on vulkan right now, early testing with phroot seems to indicate any shader that uses one just breaks currently (on shader compile) 🤔

Other issues include things like:

  • asuint etc and similar functions don't compile at all for vulkan.
  • streams.InstanceID becomes gl_InstanceID which is no longer actual and should be gl_InstanceIndex.

So the StructuredBuffer issue can probably be worked around by just using a Buffer and unpacking the data manually (like if you would pass the data in a texture, same thing anyways) and isn't urgent for this to work in mainline but would be nice to fix, at least @tebjan said they would like to have StructuredBuffer even in vulkan and might look into it.

The instance id on vulkan issue definitely needs to be fixed as it cannot be (easily?) worked around.

The asuint etc issue can probably be worked around but it's still a shame it's nonfunctional as we might end up having to blindly write code hoping it compiles correctly without the explicit cast function.

@dfkeenan
Copy link
Contributor

Regarding the shaders not automatically being included issue; have you considered doing embedded bytecode like SpriteBatch does?

@profan
Copy link
Contributor Author

profan commented Oct 16, 2019

@dfkeenan Hmm, that's not a bad idea, just need to figure out how to get to that stage 🤔

Would remove one failure mode and one less explicit package dependency, but considering one might want to change these later (or part of the shader api breaks etc) I'm hesitant to put them in such a form where it's inscrutable (or at least not easily changeable) to anyone but the most skilled reader.

@xen2
Copy link
Member

xen2 commented Oct 16, 2019

Including shader automatically (when your debug assembly is referenced) something you want?
If yes let me know, that should be possible.

@profan
Copy link
Contributor Author

profan commented Oct 16, 2019

@xen2 Aye, currently it's used in Xenko.Engine after all but it doesn't (in new or old projects) currently directly reference the debug rendering assembly (I figured it would work transitively because Xenko.Engine references it, but maybe not?) so the shaders aren't found when the shader system tries to load them.

If you do add a reference to Xenko.DebugRendering though to your created project it works without issue.

@Aggror
Copy link
Member

Aggror commented Jan 23, 2020

How is this PR going? Looking forward to seeing this in the main repo.

@profan
Copy link
Contributor Author

profan commented Jan 24, 2020

@Aggror I've been waiting for feedback from @xen2 and there are still a few changes that need to be done in Xenko itself (particularly Vulkan related ones) for this code to actually work on that backend as well that need to be done too.

I'm currently starting a new job on the 27th and right in the middle of a relocation process so I can't currently commit very much time to this at all but if I get some (any) feedback I'd be happy to get back on parts necessary.

It's just quite time consuming overall (and I also don't currently have my desktop computer as it hasn't been shipped to me yet and I can't make that happen until I get a permanent place so I don't even have a windows install I can test any of these supposed changes with).

So that's the current situation, I still do want to get this mainlined though.

@xen2
Copy link
Member

xen2 commented Apr 11, 2020

@profan
Sorry, since this PR was marked as draft it slipped under my radar and I didn't review it.
Should we attempt to merge it before rename? (which would happen within a few days)

@profan
Copy link
Contributor Author

profan commented Apr 11, 2020

@xen2 Sure, that would be good if you can do it, though I think there are more than a few things in it that need looking at, and I personally don't have a windows machine to use, likely won't have for a while (other than my work one, which I can't use), but if you want to merge it in, feel free to do whatever you want to with whats there 👀

I do think in hindsight I realised the approach I took for the debug primitives is a little bit insane? But it works and it's fast (tm).. so 👀 Just the vulkan bits, actually loading the shaders without having the users do any extra work bit that is necessary.

Also the name of the render feature is probably worth looking at, as it can be confused with the existing debug renderer maybe?

@Aggror
Copy link
Member

Aggror commented Apr 22, 2020

Will this be part of 4.0.1?

@profan
Copy link
Contributor Author

profan commented Apr 22, 2020

So, since my last comment I've ordered some fresh desktop parts and I am currently just waiting for my GPU and SSDs to arrive, then I'll be able to build a machine that can once again actually run windows (and stride) and I'll try to make the necessary changes to get this in (cleaning things up as well a bit) 👀 but if someone else wants to pick this up that's also fine 😄

@CLAassistant
Copy link

CLAassistant commented Jun 6, 2020

CLA assistant check
All committers have signed the CLA.

@profan
Copy link
Contributor Author

profan commented Jun 8, 2020

Right, so I finally vaguely received the blessing (from work) to be able to do work on stride, so I will pick this PR back up tonight and have a go at getting it in :)

@tebjan
Copy link
Member

tebjan commented Jun 8, 2020

Great to hear, nice to see you coming back!

@profan
Copy link
Contributor Author

profan commented Jun 15, 2020

So I'm back at the point where it works with Stride now again, going to go through and do some cleanup and ask xen some things about visibility groups, see if i can test with vulkan, etc 👀

@profan
Copy link
Contributor Author

profan commented Jun 17, 2020

So currently what's remaining as far as I'm aware:

Things I need input on/help with:

  • Checking if the VisibilityGroup setup is sane (is it ever recreated? how to react to it being recreated?), need @xen2 's input here.
  • The system renders to a single render group currently (31 by default, you can change it on the render system) as render objects are assigned to a single render group as well, is this fine?
  • Testing with Vulkan, can't seem to do this right now, but assuming this is being worked on with recent changes, might be fine to merge before as it should just work once done anyways?
  • Add the render feature to the default compositor asset (should it be added)?

Things I can do, but would be nice to have input on:

  • Add a stub docs page for the system so people know it exists.
  • Adding a way to toggle antialiasing of lines maybe? (as it's available in the pipeline state, not sure if would be valid on every render backend though, but still, I can do this part if we actually want it).
  • Some minor cleanup on my end (I can do this, don't need help here).

@profan profan marked this pull request as ready for review June 22, 2020 18:26
@xen2 xen2 marked this pull request as draft December 4, 2020 05:03
@jasonswearingen
Copy link
Contributor

@profan @xen2 could you please consider picking this PR back up?

Thank you

@tebjan
Copy link
Member

tebjan commented Aug 22, 2021

Yes, this is a great PR and it's partly my fault that is still open. I wanted to do a review since a long time. Just never found the time for it. But it's still on my list.

But if anyone got some time on their hands, please start the review/update to latest...

@profan
Copy link
Contributor Author

profan commented Dec 12, 2021

Adding two points to what should probably reviewed when trying to finish/merge this:

  • Vulkan compatability? (this I have not managed to test as the last time I was trying to, I couldn't get vulkan to run at all)
  • Android compatability? (afaik, someone tested an implementation of this and ran into some fun issues on android?)

@profan profan closed this Feb 7, 2023
@profan
Copy link
Contributor Author

profan commented Oct 8, 2023

Also just to be clear, I closed this PR because I thought it was never getting merged (seemed to be getting no attention with the remaining issues I wanted looked at), but someone apparently is apparently now interested in getting it in, so we'll see :)

Personally I'd be happy to see it merged, but it probably needs some changes to be compatible with latest stride?

@ArieLeo
Copy link

ArieLeo commented Oct 10, 2023

@profan are you gonna reopened the PR? having a debug renderer at runtime are usefull for visual debugging

@Eideren
Copy link
Collaborator

Eideren commented Oct 10, 2023

He'll do so if he has the time, you can fork his branch and finish the PR if you want to get this in.

@Doprez
Copy link
Contributor

Doprez commented Dec 12, 2023

I created a repo that uses the new code here and the old xenko test scripts from your original repository. I would like to move this code over to the Community Toolkit because this is too useful to go to waste.

With the toolkit the PR structure is much more relaxed and we can get this cleaned up with a seperate PR in a repository and nuget package that people can actually use without much hassle. I do see a huge benefit to having this in engine and easily accessable similar to DebugText but I think those are easy enough for most users to get the service themselves as needed for now.

I also dont see a problem with the Vulkan issue as long as we can disable it for Vulkan on exported games? I assume it wont cause issues if it isnt used, but I could be very wrong.

TLDR I want this in the toolkit but I dont want to just take your stuff without credit so I wanted to ask first.

@profan
Copy link
Contributor Author

profan commented Dec 12, 2023

I created a repo that uses the new code here and the old xenko test scripts from your original repository. I would like to move this code over to the Community Toolkit because this is too useful to go to waste.

With the toolkit the PR structure is much more relaxed and we can get this cleaned up with a seperate PR in a repository and nuget package that people can actually use without much hassle. I do see a huge benefit to having this in engine and easily accessable similar to DebugText but I think those are easy enough for most users to get the service themselves as needed for now.

I also dont see a problem with the Vulkan issue as long as we can disable it for Vulkan on exported games? I assume it wont cause issues if it isnt used, but I could be very wrong.

TLDR I want this in the toolkit but I dont want to just take your stuff without credit so I wanted to ask first.

Oh yeah by all means, please go ahead!

I'd love it if someone actually used it, I spent quite a while on it after all, so you're very welcome 👀

And re issues on exported games, I think it shouldn't be an issue if not used no, so that part should be totally fine.

Leave credit somewhere and that's all I ask :)

NOTE: the way the system generates the meshes so that we can switch them to wireframe is somewhat chaotic and could probably be replaced with a better way and a less insane shader, but it shouldn't be too hard to change those internal bits of the library as/if when necessary, just happy to see it come to use anyways!

@Doprez
Copy link
Contributor

Doprez commented Jan 3, 2024

I finally took the time to add a PR to the community toolkit. I added your name to the PR itself but if you would like credit in the comments or something else please let me know we can work with @VaclavElias to make sure everyone knows the work was done by you and not me lol.

@VaclavElias
Copy link
Contributor

Thank you. I will mention profan in our toolkit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.