-
Notifications
You must be signed in to change notification settings - Fork 313
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
Fractional element dimensions not properly rendered #438
Comments
Yeah, I have been thinking about this issue for some time. It is a bit tricky to get completely right, but I have some ideas. My general recommendation would be to try not to make your layout depend on exact pixel placement, but I understand sometimes that can be useful. |
I think to properly get scalable user interfaces there is no other way, than to get exact same result as chrome gives. That is: layout total size of 10 divs of 9.4px height, should be 94px. No more, no less. I have been looking a bit through the code, there are few places where rounding takes place: RmlUi/Source/Core/Geometry.cpp Line 106 in e407e99
Maybe it is just not needed? |
Also, I just checked, and it does not seem to be issue with layout. As in RmlUi demo, 10 divs of 9.4px height give exactly 94 px total height. And it is perfectly fine. The issue seems to be rendering really. |
Yeah, layout uses fractional pixels, the rounding happens during rendering. However, changing the rounding does not solve this issue. That will result in blurry rendering if you're using AA, or basically the same issue if you're not, since the fractional positions will placed like it was rounded. Note that there is no such thing as exact scalability with finite precision. E.g. Chrome's precision is limited to their fixed 1/64th layout pixel size. Try adding 100 divs with height 1/100px in Chrome, and I'm pretty sure it will start sweating too ;) Other issues is when you start dividing e.g. 100px sized divs with three smaller divs. Depending on rounding this may or may not fit. There is no magic bullet to this. I'm not sure, but I would guess that Chrome and Firefox does special handling of such cases. I don't think we want to go down that route, because I'm sure such special cases never end. Authors can quite simply leave some extra space, or use flexboxes or tables, which are designed to handle such situations naturally. Another thing I believe browsers do, is performing layout in CSS pixels, instead of raw pixels like we do. This is something that helps make layout more stable when scaling. I have been pondering about making that change, and also make the Of course, all of the above doesn't actually help the case you show here, because the layout is correct in this case. What we do here, is that we round the positions of each box, keeping the size of each box constant. However, what Chrome does, is rounding the corners of the box, so that the size of each box varies. You can see that if you carefully study the rendered height of each box in your example (add borders to make it visible). I have been intending to do essentially the same thing, and I do have this working locally. One complexity here is that we can move boxes around in fractional pixel. Now, this then might mean changing the rendered height of a box as you move it around. Which is kind of weird. Or maybe we just say that it takes its height based on the initial layout. I guess this turned into an essay. |
Thank you for response.
Forgive me for my stupidness, I am not rendering master. Though, I think it depends on rendering settings. With disabled AA, within my game engine I wrote following code: m_renderer->DrawRectangle( FloatRect( 0, 0, 400, 200 ), Color( 0, 0xff, 0 ) );
float currenty = 10;
float x0 = 10;
float x1 = x0 + 100;
for( int i = 0; i < 10; ++i )
{
m_renderer->DrawRectangle( FloatRect( x0, currenty, x1, currenty + 9.4f ), Color( 0xff, 0, 0 ) ); // border
currenty += 9.4f;
}
currenty = 10;
x0 = 150;
x1 = x0 + 100;
for( int i = 0; i < 10; ++i )
{
m_renderer->DrawRectangle( FloatRect( x0, currenty, x1, currenty + 1.f ), Color( 0, 0, 0 ) ); // border
currenty += 1.f;
m_renderer->DrawRectangle( FloatRect( x0, currenty, x1, currenty + 9.4f ), Color( 0xff, 0, 0 ) ); // rectangle
currenty += 9.4f;
m_renderer->DrawRectangle( FloatRect( x0, currenty, x1, currenty + 1.f ), Color( 0, 0, 0 ) ); // border
currenty += 1.f;
}
And the result on both rendering backends (dx11 and opengl) looks exactly the same as in chrome. No blurry pixels, nor the gaps between boxes. The result is sharp to death.
that is the case. One box is 9px height, other one 10px. So, maybe instead of resolving it on CPU side, let it be resolved by GPU?
Could you please push the branch with it? |
You're right, in this case it does work with AA off, however, there are issues more generally. See the related discussion, and some of the considerations made, here: I'll take a closer look to find something we can be happy with soon. Here's the previous diff if you want to play with it: diff --git a/Source/Core/ElementBackgroundBorder.cpp b/Source/Core/ElementBackgroundBorder.cpp
index 6869c234..67d0f105 100644
--- a/Source/Core/ElementBackgroundBorder.cpp
+++ b/Source/Core/ElementBackgroundBorder.cpp
@@ -98,6 +98,7 @@ void ElementBackgroundBorder::GenerateGeometry(Element* element)
{
Vector2f offset;
const Box& box = element->GetBox(i, offset);
+ offset += element->GetAbsoluteOffset(Box::BORDER) - element->GetAbsoluteOffset(Box::BORDER).Round();
GeometryUtilities::GenerateBackgroundBorder(&geometry, box, offset, radii, background_color, border_colors);
}
diff --git a/Source/Core/ElementStyle.cpp b/Source/Core/ElementStyle.cpp
index ef103ad4..22c1a275 100644
--- a/Source/Core/ElementStyle.cpp
+++ b/Source/Core/ElementStyle.cpp
@@ -647,16 +647,16 @@ PropertyIdSet ElementStyle::ComputeValues(Style::ComputedValues& values, const S
break;
case PropertyId::BorderTopWidth:
- values.border_top_width = ComputeLength(p, font_size, document_font_size, dp_ratio);
+ values.border_top_width = Math::RoundFloat(ComputeLength(p, font_size, document_font_size, dp_ratio));
break;
case PropertyId::BorderRightWidth:
- values.border_right_width = ComputeLength(p, font_size, document_font_size, dp_ratio);
+ values.border_right_width = Math::RoundFloat(ComputeLength(p, font_size, document_font_size, dp_ratio));
break;
case PropertyId::BorderBottomWidth:
- values.border_bottom_width = ComputeLength(p, font_size, document_font_size, dp_ratio);
+ values.border_bottom_width = Math::RoundFloat(ComputeLength(p, font_size, document_font_size, dp_ratio));
break;
case PropertyId::BorderLeftWidth:
- values.border_left_width = ComputeLength(p, font_size, document_font_size, dp_ratio);
+ values.border_left_width = Math::RoundFloat(ComputeLength(p, font_size, document_font_size, dp_ratio));
break;
case PropertyId::BorderTopColor:
diff --git a/Source/Core/GeometryBackgroundBorder.cpp b/Source/Core/GeometryBackgroundBorder.cpp
index 7bf459f4..cb7de1fd 100644
--- a/Source/Core/GeometryBackgroundBorder.cpp
+++ b/Source/Core/GeometryBackgroundBorder.cpp
@@ -37,7 +37,7 @@ namespace Rml {
GeometryBackgroundBorder::GeometryBackgroundBorder(Vector<Vertex>& vertices, Vector<int>& indices) : vertices(vertices), indices(indices)
{}
-void GeometryBackgroundBorder::Draw(Vector<Vertex>& vertices, Vector<int>& indices, CornerSizes radii, const Box& box, const Vector2f offset, const Colourb background_color, const Colourb* border_colors)
+void GeometryBackgroundBorder::Draw(Vector<Vertex>& vertices, Vector<int>& indices, CornerSizes radii, const Box& box, Vector2f offset, const Colourb background_color, const Colourb* border_colors)
{
using Edge = Box::Edge;
@@ -57,7 +57,7 @@ void GeometryBackgroundBorder::Draw(Vector<Vertex>& vertices, Vector<int>& indic
num_borders += 1;
}
- const Vector2f padding_size = box.GetSize(Box::PADDING).Round();
+ Vector2f padding_size = box.GetSize(Box::PADDING);
const bool has_background = (background_color.alpha > 0 && padding_size.x > 0 && padding_size.y > 0);
const bool has_border = (num_borders > 0);
@@ -65,9 +65,11 @@ void GeometryBackgroundBorder::Draw(Vector<Vertex>& vertices, Vector<int>& indic
if (!has_background && !has_border)
return;
+ Math::SnapToPixelGrid(offset, padding_size);
+
// -- Find the corner positions --
- const Vector2f border_position = (offset + box.GetPosition(Box::BORDER)).Round();
+ const Vector2f border_position = offset;
const Vector2f padding_position = border_position + Vector2f(border_widths[Edge::LEFT], border_widths[Edge::TOP]);
const Vector2f border_size = padding_size + Vector2f(border_widths[Edge::LEFT] + border_widths[Edge::RIGHT], border_widths[Edge::TOP] + border_widths[Edge::BOTTOM]); |
@mikke89 I have tried to apply patch you have given, but with no effects. I have fought with this problem in recent days, and here are my thoughts on it (and Draft #502).
When I first read it few months ago I wasn't really aware of what it means. class RMLUICORE_API RenderInterface /* .... */ {
....
/// Called by RmlUi when it wants to compile geometry it believes will be static for the forseeable future.
/// If supported, this should return a handle to an optimised, application-specific version of the data. If
/// not, do not override the function or return zero; the simpler RenderGeometry() will be called instead.
/// @param[in] vertices The geometry's vertex data.
/// @param[in] num_vertices The number of vertices passed to the function.
/// @param[in] indices The geometry's index data.
/// @param[in] num_indices The number of indices passed to the function. This will always be a multiple of three.
/// @param[in] texture The texture to be applied to the geometry. This may be nullptr, in which case the geometry is untextured.
/// @return The application-specific compiled geometry. Compiled geometry will be stored and rendered using RenderCompiledGeometry() in future
/// calls, and released with ReleaseCompiledGeometry() when it is no longer needed.
virtual CompiledGeometryHandle CompileGeometry(Vertex* vertices, int num_vertices, int* indices, int num_indices, TextureHandle texture); When Rml already created CompiledGeometry then the only thing it can do afterwards is to render it and destroy: /// Called by RmlUi when it wants to render application-compiled geometry.
/// @param[in] geometry The application-specific compiled geometry to render.
/// @param[in] translation The translation to apply to the geometry.
virtual void RenderCompiledGeometry(CompiledGeometryHandle geometry, const Vector2f& translation);
/// Called by RmlUi when it wants to release application-compiled geometry.
/// @param[in] geometry The application-specific compiled geometry to release.
virtual void ReleaseCompiledGeometry(CompiledGeometryHandle geometry); Consider these simple red boxes and border around them:
That is: one box has height 9px, other one 10px. It all depends on where geometry is finally placed on screen. Once we upload data to GPU, it cannot be really modified afterwards by Rml. So it is hard to decide whether to upload 10 px or 9 px high. This issue cannot be resolved on CPU as long as we have CompiledGeometry, and as long as CPU cannot modify CompiledGeometry GPU data (though it might be heavy operation on performance). It is simply not possible. (I removed rounding from CPU in commit in Draft 34bd460) In previous messages I suggested to let GPU handle this whole topic. Without antialiasing, it works pretty well. Though, issues are visible at the first glance: blurry fonts and inconsistent borders: .container {
display: flex;
flex-wrap: wrap;
width: 100%;
height: 100%;
}
.item {
width: 103px;
height: 49.4px;
background-color: red;
border: 10px black;
border-radius: 10px;
margin: 9.49992px;
} <div class="container">
<div class="item"></div>
<div class="item"></div>
<div class="item"></div>
<div class="item"></div>
<div class="item"></div>
....
</div> It happens because we let GPU handle all things on itself. And GPU is too stupid to do it properly. Instead of rounding everything on CPU, let's do rounding in vertex shader on GPU:
Fonts are sharp now. But still, there are problems with inconsistent borders. Whenever we attempt to draw rounded corners of border, we must let GPU know it needs special handling. And it must be per vertex basis. struct RMLUICORE_API Vertex {
/// Two-dimensional position of the vertex (usually in pixels).
Vector2f position;
/// RGBA-ordered 8-bit / channel colour.
Colourb colour;
/// Texture coordinate for any associated texture.
Vector2f tex_coord;
/// Relative position (helps with properly snapping vertices for sharp looking)
Vector2f relative_position;
}; And let's set it only when we draw corners of border: void GeometryBackgroundBorder::DrawArc(Vector2f pos_center, Vector2f r, float a0, float a1, Colourb color0, Colourb color1, int num_points)
...
for (int i = 0; i < num_points; i++)
const float t = float(i) / float(num_points - 1);
const float a = Math::Lerp(t, a0, a1);
const Vector2f unit_vector(Math::Cos(a), Math::Sin(a));
vertices[offset_vertices + i].position = unit_vector * r + pos_center;
vertices[offset_vertices + i].relative_position = ... You only need to pick that point wisely (red one) In vertex shader handling is pretty simple: just calculate offset between snapped relative_position and relative_position, and add it to vertex position (don't snap it!) Now all borders are properly rendered, there are no inconsistencies. You can also see, that initial issue is also fixed: These all changes are made in one commit: 6656940 I also checked how it all behaves under Multisample Antialiasing (MSAA). It looks very decent I would say. Having in mind that all is rendered with fractional dimensions it is quite cool. To get this all right will take time. We would have to implement custom shaders in all backends. I have played around with this solution, threw at it different html+css, and it all looks very good.
@mikke89 I would really appreciate if you look at it and try to provide example that will break whole solution, and it cannot be fixed. |
@mwl4 First of all, I love the effort that went into this. I will need some time to go through it and get back to you :) |
I have brought improvements in next commit: a940127 I slightly changed vertex which I pick as relative one, as instead of red one, I pick green one (for rounded corner). Now, I believe all scenarios for borders are supported properly: (screenshots come from version without AA) border: 10px black;
border-radius: 10px;
border: 20px black;
border-radius: 10px;
border: 10px black;
border-radius: 15px;
Of course, fractional values are also allowed and rendered properly: border: 11.6px black;
border-radius: 14.7px;
I played with different settings, and not everytime all corners are consistent. I think what we achieve is really decent result. |
I've taken a closer look at this one now, and I really like the visual result of your approach here. However, there are some aspects of it that I'm not thrilled about:
With that said, your efforts show that you are serious about this, and I want to work together to find a solution.
This is not really about CompiledGeometry fundamentally, although that is one aspect we would have to deal with. This is more about the implications of having the visual height of the element changed, by modifying the element's position. This is just something that needs to be considered for when to (re-)generate background geometry.
You're right that we cannot modify CompiledGeometry directly, however, there is nothing stopping us from regenerating the geometry. And in fact, I think that is exactly something we should consider. Here is what I suggest: Consider that the background size can change, not only from its layout size, but also when its position changes. After we change the position or size of the element (its boxes), we dirty some kind of background size flag. During rendering, we check this flag, and calculate the new rounded background size. This new size is compared to the old one, and if it changed, we remake the background geometry and decorators. Let me draw out an example: That is, the background (including borders, and decorators) would always be drawn at integer pixel size. Their origin should be zero, and we round the position when submitting the render call. The rounded size becomes Border widths and radius should be rounded, even at the layout level (when finding computed values), actually I believe I already made this change on master. This should simplify rendering of borders, especially in terms of finding their inner edge, and I believe this is what all the web browsers are doing already. This should be completely backwards compatible, and not require any additional vertex data. The obvious drawback is that we may need to regenerate the geometry in more situations. However, this might not be much more often actually: We already generate new geometry after doing layout, and that needs to be done with any properties affecting size, or adding new elements, etc. The only time when we didn't is when changing the position of absolutely or relatively positioned elements. I'd say usually this is not done too often, and when it is done, it will certainly be cheaper than a layout step in any case, so to me this sounds acceptable. Further, if either the position or size is always integer, then the rounded size will not change. What do you think? Would you be willing to prototype this, and see how it works out? |
I would say logic in vertex shader is quite simple. It requires simple if statement and proper rounding. With proper documentation it will be hard to get it wrong. On the other hand, code on CPU side simplifies a lot. You don't have to think anymore if you need to round/snap position while layouting or not. It would all be handled when rendering. I take it as huge advantage after removing all these rounds and snaps around the code, as they appear in really weird scenarios.
Yes and no. If UI draws 100k vertices (which I take very unlikely, and it would require very complex UI) - additional memory usage would be just 0.76 MB. For modern GPUs it's nothing (comparable to one small image uploaded to GPU for drawing). If we consider data transfer from CPU to GPU, then we need to remember that we are dealing with CompiledGeometry, which is uploaded only on creation.
OpenGL 2 supports shaders, so that should not be problem. SDLrenderer is kind of problem. I think if you want to develop RmlUI further, you have to drop support for SDL renderer. Though, even right now, getting support for it in SDL renderer is quite easy. As we do not support CompiledGeometry in SDL renderer, we already do something similiar with void RenderInterface_SDL::RenderGeometry(Rml::Vertex* vertices, int num_vertices, int* indices, int num_indices, const Rml::TextureHandle texture,
const Rml::Vector2f& translation)
{
SDL_FPoint* positions = new SDL_FPoint[num_vertices];
for (int i = 0; i < num_vertices; i++)
{
positions[i].x = vertices[i].position.x + translation.x;
positions[i].y = vertices[i].position.y + translation.y;
}
SDL_Texture* sdl_texture = (SDL_Texture*)texture;
SDL_RenderGeometryRaw(renderer, sdl_texture, &positions[0].x, sizeof(SDL_FPoint), (const SDL_Color*)&vertices->colour, sizeof(Rml::Vertex),
&vertices->tex_coord.x, sizeof(Rml::Vertex), num_vertices, indices, num_indices, 4);
delete[] positions;
} Just replace positions[i].x = vertices[i].position.x + translation.x;
positions[i].y = vertices[i].position.y + translation.y; with equivalent of vertex shader logic and done. The solution you suggest might introduce performance issues, because in some situations you will have to upload data to GPU very often: scrollable area for example? |
If I had to choose, I'd rather have this complexity on the library side, so we don't have to explain it to users at all, or require any changes from them. And I believe my proposal is not very complicated really.
Yeah, we might want to have a look at those roundings, although this applies regardless of which of the solutions we go for. Just note that there might be some layout implications for doing rounding. E.g. web browser do table layouting in integers to ensure that the width is properly distributed among the columns. We have to ensure that this works as expected if we remove these roundings. And this goes for flexbox layout too.
Scrolling is always rounded already, so it shouldn't change any of the box sizes as the user is scrolling around. Your points are generally valid, but I still believe the different trade-offs favor the proposed solution. In any case a prototype would be more informative, I'd love to see one if you are willing to implement it. |
Hi,
I am having issue with fractional element dimensions.
Reproduction:
Samples/demo.exe
Expected result: (chrome)
https://jsfiddle.net/mfv29rxt/
You might wonder, why I need fractional dimensions. In most of the time you want to achieve scalable user interface, so you use
hv
ordp
units. Then fractional dimensions are calculated by library.The text was updated successfully, but these errors were encountered: