-
Notifications
You must be signed in to change notification settings - Fork 9
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
Hatch fill patterns for the CAD example #116
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 5629f9f.
62_CAD/DrawResourcesFiller.cpp
Outdated
@@ -280,44 +279,50 @@ void DrawResourcesFiller::drawHatch(const Hatch& hatch, const float32_t4& color, | |||
drawHatch(hatch, color, InvalidTextureHash, intendedNextSubmit); | |||
} | |||
|
|||
void DrawResourcesFiller::addMSDFTexture(ICPUBuffer const* srcBuffer, const asset::IImage::SBufferCopy& region, texture_hash hash, SIntendedSubmitInfo& intendedNextSubmit) | |||
void DrawResourcesFiller::addMSDFTexture(ICPUBuffer const* srcBuffer, uint64_t bufferOffset, uint32_t3 imageExtent, texture_hash hash, SIntendedSubmitInfo& intendedNextSubmit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change ICPUBuffer const*
here and in TextureCopy struct to smart_refctd_ptr
because we're holding onto the data until we finalize the copies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
IGPUCommandBuffer::SPipelineBarrierDependencyInfo::image_barrier_t barriersBeforeCopy[] = | ||
using image_barrier_t = IGPUCommandBuffer::SPipelineBarrierDependencyInfo::image_barrier_t; | ||
std::vector<image_barrier_t> barriers; | ||
barriers.reserve(textureCopies.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you decided to do all in 1 barrier.
But remember there was a bug you even found out and mentioned to me before:
If you barrier from srcLayout=UNDEFINED the contents may be lost (implementation dependant based on vk docs) so you may lose other texture contents when uploading only 1 idx
so let's do it one by one for those you only want to copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
62_CAD/DrawResourcesFiller.h
Outdated
@@ -24,6 +25,24 @@ struct DrawBuffers | |||
smart_refctd_ptr<BufferType> lineStylesBuffer; | |||
}; | |||
|
|||
// TODO: Better place for this | |||
enum class MsdfFillPattern: uint32_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more high-level than DrawResourcesFiller, so make that and the factory functions that give you msdfs into a seperate file. this way we can easily copy paste into n4ce and use that there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have classes/structs related to text placement and msdf generations anyways, it makes sense to put them all in there along with the hashing of the data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When doing it one by one, the validation layer kept claiming that the resource was on the wrong layout, we could make it transition from shader read, and only use undefined for the first transition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UNDEFINED is not a wrong layout.
Your next barrier after the upload was the issue. Because some of the textures were freshly TRANSFER_DST and some of them were old SHADER_READONLY
Doing both the before and after barriers for only the uploading ones shouldn't result in a validation error.
Any transition barrier chain which starts from UNDEFINED, should be pretty straightforward because you're completely overwriting the previous slot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (I think commented on the wrong place here)
62_CAD/main.cpp
Outdated
LRUCache<int, char> hugeCache(50000000u); | ||
hugeCache.insert(0, '0'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old test code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
62_CAD/main.cpp
Outdated
{ | ||
for (int x = 0; x < msdfMap.width(); ++x) | ||
{ | ||
auto pixel = msdfMap(x, glyphH - 1 - y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK this msdfgen::pixelFloatToByte
is a multiplication by 256
so UNORM turns to UINT.
but your msdf textures are UNORM... something doesn't feel right here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok my bad, brainfart! it should be filled with uint8_t
BUTTTT you can probably leverage format conversions WHILE uploading with our image upload utility. (change srcFormat to RGBA8F in the input params)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devshgraphicsprogramming can you verify this? I forgot the vk format stuff regarding conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
62_CAD/MSDFs.h
Outdated
break; | ||
} | ||
|
||
return textureTypeHash ^ (textureHash << 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use core::hash_combine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
62_CAD/MSDFs.h
Outdated
textureHash = std::hash<uint32_t>{}(uint32_t(s.fillPattern)); | ||
break; | ||
case MsdfTextureType::FONT_GLYPH: | ||
textureHash = std::hash<uint32_t>{}(s.glyphIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of glyph index, take glyphHash which will be hashed by our text glyph generator tool later on. (it depends also on the font and the font settings and flags you pass into FT to generate those glyphs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
62_CAD/MSDFs.h
Outdated
textureHash = std::hash<uint32_t>{}(uint32_t(s.fillPattern)); | ||
break; | ||
case MsdfTextureType::FONT_GLYPH: | ||
textureHash = std::hash<uint32_t>{}(s.glyphIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then this path becomes textureHash = s.glyphHash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
62_CAD/DrawResourcesFiller.h
Outdated
uint32_t3 imageExtent; | ||
std::vector<CPolyline> polylines; | ||
}; | ||
void addMSDFTexture(std::function<MsdfTextureUploadInfo()> createResourceIfEmpty, texture_hash hash, SIntendedSubmitInfo& intendedNextSubmit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few edits I suggest:
- function returns the index/slice in our texture we have allocated. this way we don't have to look up in the cache if we need to work with the index immediately (for example text)
- Lambda is good, another choice is to
getMSDFTextureIndex(hash)
so it can be looked up from outside, yours is better in a way that avoids checking the cache twice (one from outside, one inside in "cache->insert") - Reuse the Lambda function inside the other one to avoid duplicate code. we might want to use that as a convinience function later because for the 32x32 msdf hatch fill shapes it makes more sense to keep them around in cpu memory in a CPUBuffer array, ready to use and not have to go through the whole msdf generation everytime we can't find it in cache (it's fine in the example rn, I'm talking about n4ce codebase)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
62_CAD/MSDFs.cpp
Outdated
float scaleY = (1.0 / float(glyphExtents.y)) * glyphH; | ||
msdfgen::generateMTSDF(msdfMap, glyph, MsdfPixelRange, { scaleX, scaleY }, msdfgen::Vector2(0.0, 0.0)); | ||
|
||
// TODO: Optimize this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per discussions in discord, you can remove the for loops and set srcFormat in image upload utility to RGBA8_SFLOAT to get the conversion while memcpying to staging memory with our image filters (all happening inside the util)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
62_CAD/fragment_shader.hlsl
Outdated
return max(min(r, g), min(max(r, g), b)); | ||
} | ||
|
||
float screenPxRange(float2 uv) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain this function to me?
more explicitly why divide by float2(globals.resolution);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's basically this:
On screen size of the quad
/ MSDF Resolution
* MSDF Pixel Range
(From msdfgen docs: screenPxRange()
represents the distance field range in output screen pixels. For example, if the pixel range was set to 2 when generating a 32x32 distance field, and it is used to draw a quad that is 72x72 pixels on the screen, it should return 4.5 (because 72/32 * 2 = 4.5). For 2D rendering, this can generally be replaced by a precomputed uniform value.)
I did it using the 3D path as we have to support transformations on the text (due to DWG and such), so this allows it to be more flexible (and only costs one fwidth)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be / float2(32, 32); or the msdf texture size instead of float2(globals.resolution);
let's double check the math, so we're trying to get from
-
from [0, 1] of msdf sampled value to [-0.5, 0.5] by
(sd - 0.5)
-
then we multiply by
MsdfPixelRange
to get it to [-MsdfPixelRange/2, MsdfPixelRange/2] and that is basically the
distance in glyph texture units (because the spectrum/range from lowest to highest sdf is MsdfPixelRange! the input to msdfgen!)
now we need this distance in uv units: -
we divde by (32,32) or
GetDimensions(msdfTexture)
to get the distance in uv units -
now, how to turn uv distance into actual screenspace distance?! that's the main question!
here it uses fwidth to do that because it shows you the rate of change of both U and V with regards to x and y on the screen and the inverse of that gives you the screenspace size of the
so if everything is aligned (u with x and v with y) it becomes 1.0 / abs(float2(ddx(u),ddy(v)))
1/ddx(u)
gives you the horizontal size of the glyph box in screen pixels!
and 1/ddy(v)
gives you the vertical size of the glyph box in screen pixels!
If you're not sure why read the thought experiment below:
++++
Optional Read
ddx(u) tells you the rate of change of u per pixel in x direction
if you think about it ddx(u) is basicaly dU/dScreenPixel
(if you go 1 pixel how much does u value change?)
and 1/ddx(u) is dScreenPixel/dU
and gives you the inverse rate of change (if you go by 1 units in u
how many units in screen pixels? ---> width pixels!)
++++
But fwidth is just used as an approximation. float2 fwidth = abs(ddx(uv) + ddy(uv))
small note: ddx(uv) is a float2, it's computing ddx for both u and v.
Now that you understand this, you get why is the / float2(globals.resolution)
is buggy and why you need to have different functions for this for 1. glyph boxes 2. hatch fills
and if you fix this you are going to see these bugs fixed:
- some hatch fills msdfs being buggy or non connected
- text being too blurry when zooming in
A. How to handle this function for "Hatch Box Fills"?
for hatch box fills we cannot use this fwidth(uv)
because we're doing repeat sampling of a 8x8 msdf onto the screen! (that's ptobably you caused a double bug by the wrong division, two wrongs sometimes make a right :D)
so for hatch fills it's float2 screenTexSize = float2(8,8)
or whatever the fixed size for them is!
B. How to handle this function for "glyph boxes"?
for glyph boxes (or I think you call the fontglyphs in other branch), the screenspace size of the glyph box can be computed in vshader and get passed along! no need for fwidth, and it will result in a nice shape with any rotation because the sdf would be exact as the last step (4th one) above get's you correctly from uv distance to screenspace distance, then you can use that to compute opacity or whatever
62_CAD/MSDFs.cpp
Outdated
|
||
float scaleX = (1.0 / float(glyphExtents.x)) * glyphW; | ||
float scaleY = (1.0 / float(glyphExtents.y)) * glyphH; | ||
msdfgen::generateMTSDF(msdfMap, glyph, MsdfPixelRange, { scaleX, scaleY }, msdfgen::Vector2(0.0, 0.0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you generating MTSDF (it has true sdf in alpha channel) but don't use it? you can just make the MSDF textures RGB and generateMSDF instead of MTSDF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was for debugging, easier to know what's wrong with the texture in render doc that way, but I'll change it over when everything is working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Msdfs are harder to understand and read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you can just use msdf.exe, generate the msdf with cmdline and compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't use that with the hatch fills, since they're not exactly font files, but I'll change it over to msdf only when everything works
…es to not bother with DLLs, mark TODO regarding Nabla Text Rendering extension for future
…3rpdarty freetype + msdfgen-ext directly without the extension enabled
// TODO needs to handle rotations as well, probably from curve box code | ||
const float2 ndcAxisMin = screenTopLeft; | ||
const float2 ndcAxisMax = screenTopLeft + screenDirU + screenDirV; | ||
const float2 screenSpaceAabbExtents = abs(ndcAxisMax - ndcAxisMin) * float2(globals.resolution); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use lenght(screenDirU) and length(screenDirV) , to get the length of the sides
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, not aabb anymore. Use box or obb
const float2 screenDirV = (float2) transformVectorNdc(clipProjectionData.projectionToNDC, dirV); | ||
|
||
float2 corner = float2(bool2(vertexIdx & 0x1u, vertexIdx >> 1)); | ||
const float2 coord = screenTopLeft + corner * (screenDirU + screenDirV); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be topLeft + corner.x * dirU + corner.y * dirV
Now rotation is handled
@@ -372,6 +372,23 @@ float32_t4 calculateFinalColor<true>(const uint2 fragCoord, const float localAlp | |||
return col; | |||
} | |||
|
|||
// TODO: place this MSDF code elsewhere |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be in nbl/hlsl/text_rendering/msdf.hlsl within correct namespace.
(Fix indentation please with the brackets)
Do this when you've finalised working on these functions cause otherwise built-ins have to rebuild each time you make a change to the hlsl built-in.
Don't forget to document what the different input params and terms such as msdfPixelRange means and how it's related to the msdfgen lib (you can copy paste and quote from the lib above these functions)
return max(min(r, g), min(max(r, g), b)); | ||
} | ||
|
||
float screenPxRange(float2 screenSpaceSizePixels, float2 distanceFieldSizePixels) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not bring this function to the builtin hlsl header, because it's application dependant. Let's be flexible like msdfgen about user providing a screenPxRange. But remember to document clearly so there is no confusion about what screenPxRange is
It is the value used to convert the distance values in the msdf texture to distance in screen space. In other words it's
DistanceInScreenSpace/DistanceInMSDFTextureSpace
, the larger the glyph (or more zoomed in) the larger this value is.
@@ -197,6 +211,8 @@ NBL_CONSTEXPR uint32_t InvalidTextureIdx = nbl::hlsl::numeric_limits<uint32_t>:: | |||
NBL_CONSTEXPR MajorAxis SelectedMajorAxis = MajorAxis::MAJOR_Y; | |||
// TODO: get automatic version working on HLSL | |||
NBL_CONSTEXPR MajorAxis SelectedMinorAxis = MajorAxis::MAJOR_X; //(MajorAxis) (1 - (uint32_t) SelectedMajorAxis); | |||
NBL_CONSTEXPR float MsdfPixelRange = 4.0; | |||
NBL_CONSTEXPR float MsdfSize = 32.0; // TODO: Should this be dynamic? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, but make sure to use MSDF all caps + use that on the c++ side as well
float msdfOpacity(float3 msd, float2 screenPxRangeValue) { | ||
float sd = median(msd.r, msd.g, msd.b); | ||
float screenPxDistance = screenPxRangeValue * (sd - 0.5); | ||
float opacity = clamp(screenPxDistance + 0.5, 0.0, 1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't output opacity, output screen space distance and do AA with smooth step like we already do.
This is literally a sdf function, but reads from texture
TODO list: