godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.2k stars 21.21k forks source link

Vulkan: Support drawing thin/Bresenham lines on NVIDIA #75654

Open PureAsbestos opened 1 year ago

PureAsbestos commented 1 year ago

Godot version

4.0.1

System information

Windows 10, Vulkan, NVIDIA GeForce GTX 1070

Issue description

Not sure if this is technically a bug, but I consider it a regression introduced in Godot 4 with Vulkan. Line rendering (e.g. with draw_line, Line2D, etc.) relies on the graphics backend, and the default behavior for line rendering is different between OpenGL and Vulkan. OpenGL uses the familiar Bresenham style lines, while Vulkan uses something a bit different.

vulkan-opengl-lines Even in OpenGL compatibility mode, the lines are drawn in the Vulkan style. The OpenGL behavior was preferable for many use-cases, and is especially important for pixel art games and pixel art editors (which is what I'm making at the moment).

It seems that the best solution to this might be to use VK_EXT_line_rasterization to (optionally?) provide this behavior, though as I understand it there might be some portability issues there (I don't think MoltenVK supports VK_EXT_line_rasterization yet for instance).

Update: this problem does not exist consistently everywhere. This new line-drawing style shows up with my NVIDIA GPU but not with Intel Integrated Graphics, which still uses Bresenham-style lines. It really ought to be consistent across devices.

Steps to reproduce

Use draw_line in Godot 3 Use draw_line in Godot 4 and notice that it looks different

Minimal reproduction project

Godot 3 project: line-test-3.zip

Godot 4 project: line-test.zip

AThousandShips commented 1 year ago

Are you talking different lines in 4.0 and 3.x or between Forward+ and compatibility?

PureAsbestos commented 1 year ago

Between 4.0 and 3.x. Forward+ and Compatibility both use Vulkan style line rendering

AThousandShips commented 1 year ago

That is weird as the compatibility backend is OpenGL, not Vulkan, don't know what it is doing

AThousandShips commented 1 year ago

Unfortunately VK_EXT_line_rasterization is not very widely supported it seems, so not sure how likely this is to be supported, considering relatively minor visual difference

PureAsbestos commented 1 year ago

That is weird as the compatibility backend is OpenGL, not Vulkan, don't know what it is doing

My best guess would be that Compatibility is doing something like translating Vulkan code.

Unfortunately VK_EXT_line_rasterization is not very widely supported it seems, so not sure how likely this is to be supported, considering relatively minor visual difference

It's a shame it's not yet very widely supported. The visual difference may seem minor, but to me and perhaps many others it is essential. I will roll my own line renderer or something rather than settle for this (which is both a pain to do and perhaps significantly less performant).

dalexeev commented 1 year ago

It probably depends on the OS/driver. I can't reproduce this.

Vulkan API 1.3.224 - Forward+ - Using Vulkan Device #0: Intel - Intel(R) UHD Graphics 630 (CML GT2)

https://user-images.githubusercontent.com/47700418/229897469-8fe4ca25-2170-4a01-93d3-e8e3589584f5.mp4


EDIT: You probably meant thick lines (width > 0):

https://user-images.githubusercontent.com/47700418/229899992-956017e7-1dae-4306-aac9-1aaef593ab34.mp4

PureAsbestos commented 1 year ago

Ooh, that's interesting... I thought it was defined behavior in the spec. No, this happens for me with the default width = -1.0 as well

dalexeev commented 1 year ago

No, this happens for me with the default width = -1.0 as well

This is weird, it works for me correctly for width < 0.

Note that in 3.x this also doesn't work for thick lines (width > 1.001 - see #69851 for details).

PureAsbestos commented 1 year ago

I am going to update my graphics drivers to see if that helps, but it would be a tad odd if the behavior of my app relied on having up-to-date drivers... I'd also like to try switching the Vulkan device to integrated but not sure how to do that right now.

dalexeev commented 1 year ago

it would be a tad odd if the behavior of my app relied on having up-to-date drivers...

PureAsbestos commented 1 year ago

Okay, I can confirm the problem still exists with the latest NVIDIA drivers installed.

PureAsbestos commented 1 year ago

With a bit of work I was able to get it to launch with Intel Integrated Graphics, and there it does indeed use Bresenham-style lines...

Calinou commented 1 year ago

I suggest documenting this caveat for now, as support for the extension isn't great:

image

Calinou commented 1 year ago

This part surprises me though:

Even in OpenGL compatibility mode, the lines are drawn in the Vulkan style.

I'm fairly sure the code used for line drawing is identical between 3.x GLES3 and 4.0 Compatibility, so unless thin lines are being drawn using polygons, this shouldn't happen.

@PureAsbestos Please upload a minimal reproduction project to make this easier to troubleshoot.

AThousandShips commented 1 year ago

Based on the current code it does not draw polygons with thin lines: https://github.com/godotengine/godot/blob/44d539465acca7592e0c88748e231fe5f151da37/servers/rendering/renderer_canvas_cull.cpp#L611-L620

PureAsbestos commented 1 year ago

@Calinou added two, one in Godot 4 showing the issue, and one in Godot 3 showing the previous behavior

PureAsbestos commented 1 year ago

Oddly, compatibility mode seems to now be rendering lines the old way. Maybe it didn't properly switch or something, not sure

clayjohn commented 1 year ago

Line rendering is done by the hardware, so it is up to the hardware vendor how to implement it in the GPU driver. We don't control it in 3.x or in 4.x. VK_EXT_line_rasterization is really poorly supported, so from our end there isn't much reason to expose it.

Currently (in 3.x and 4.x) if you rely on having bresenham-exact lines, then you need to rasterize your lines on the CPU. While this isn't as efficient as GPU-rendering, it is the only way to have complete control over your rendering pipeline and not find yourself reliant on GPU drivers.

PureAsbestos commented 1 year ago

VK_EXT_line_rasterization is really poorly supported, so from our end there isn't much reason to expose it.

Well the Bresenham lines feature does appear to be supported on, e.g. modern NVIDIA cards which happen to also be the ones posing the problem. But I can understand this position.

We don't control it in 3.x or in 4.x

While the behavior isn't directly controlled by Godot in 3.x, it does behave in a consistent manner across platforms because the OpenGL spec is stricter in this area. But I agree that this is ultimately more a failing of Vulkan than Godot...

... you need to rasterize your lines on the CPU

Yep, I've come to the same conclusion (though I suppose Compatibility could work...), but it kind of sucks, especially for other applications that rely more on realtime graphics.

Looking forward to #74714 and perhaps a proper solution in the future if VK_EXT_line_rasterization becomes sufficiently supported or a good workaround is found.

Calinou commented 1 year ago

I wonder if implementing this extension will resolve https://github.com/godotengine/godot/issues/37016. Nobody has been able to reproduce this issue on AMD or Intel so far, so that checks out.

NeedsMoar commented 1 year ago

On a 7000 series AMD cards (and I'm fairly sure Vega and 6000 series), vulkaninfo shows the following, FYI:

VkPhysicalDeviceLineRasterizationFeaturesEXT:

    rectangularLines         = false
    bresenhamLines           = true
    smoothLines              = false
    stippledRectangularLines = false
    stippledBresenhamLines   = true
    stippledSmoothLines      = false

I think it's been like this since Vega at least, I'm not going to install my old HD7970 in something to check on that if it even has vulkan drivers. :-)

PureAsbestos commented 1 year ago

Device coverage has increased a little bit. image We should also consider how many of these unsupported devices would actually be supported by Godot anyway (a lot of them use old API versions. Feel free to correct this, but as I understand it, Godot requires an API version of 1.2 or higher. Also from what I can tell some devices appear to be listed in both the supported and unsupported categories, possibly due to drivers updating to support it).

Though this would be the topic for another issue, it might make some sense to have an option that enables "strict lines" (these are the Vulkan-style lines I mentioned, and while I don't like them and they are often undesirable, it makes some sense to have this option since all Vulkan devices must support them, so the output would at least be consistent across platforms).

But by default it would perhaps be best to simply enable the extension if it is available. Then the few devices which are supported by Godot, don't support VK_EXT_line_rasterization, and don't use bresenham for non-strict lines, can simply be ignored and use whatever their default line drawing behavior is (we can document the possibility of this happening) as it is likely a tiny subset of devices.

Is there any reason why the extension couldn't be enabled for platforms that support it?

Calinou commented 1 year ago

Feel free to correct this, but as I understand it, Godot requires an API version of 1.2 or higher.

Godot currently only requires Vulkan 1.0 (though in practice, devices that don't support at least Vulkan 1.1 have pretty broken support of the API in general).

Is there any reason why the extension couldn't be enabled for platforms that support it?

I agree with this on principle, but this has a performance penalty and needs to be benchmarked. Some people rely heavily on thin line drawing for debug tools (where performance does matter when you draw thousands of lines every frame).

PureAsbestos commented 1 year ago

Good to know. If it does have an appreciable performance penalty, I suppose it would be better to have as an option (say in project settings). Then for applications where performance is critical, it could be disabled, whereas for applications that prize consistent nice looking lines, it could be enabled when available. I suppose that decision would be made after benchmarking though.

I might look into making a fork/PR tentatively implementing this, though I am not so certain I am up to the task.

PureAsbestos commented 1 month ago

An interesting alternative(?) to using VK_EXT_line_rasterization for this: VK_IMG_relaxed_line_rasterization

clayjohn commented 1 month ago

An interesting alternative(?) to using VK_EXT_line_rasterization for this: VK_IMG_relaxed_line_rasterization

That looks very interesting. For context though the "IMG" in the name means it's not an official extension. It's a "vendor extension" meaning it's something made by IMGTec and it's only for their devices. So it can't be used on non IMGTec devices.