[Development] Recommendations on a patch code review

Stan Morris pixelgrease at gmail.com
Wed Oct 30 20:52:41 CET 2024


I'm working on my first patch to the Qt framework which I hope to get into
Qt 6.9. It's currently rejected, but there's a chance I can fix issues and
get it approved.

There's an important question that I don't know the answer to:

Does anyone see modifying vertex counts in a QSGGeometryNode while
animating as valuable?

If no one else wants this, I'm wasting reviewers' time and this
contribution is worth little to the community.

The patch enables resizing geometries in a QSGGeometry node without
invalidating existing geometries. Normally, resizing to add vertices
requires updating all existing geometry vertices as well as the new ones.
Resizing to remove vertices requires updating the remaining vertices.

Suppose a geometry node has 1000 vertices and 10 more are added. Currently,
resizing the geometry invalidates the entire buffer and all previous 1000
vertices must be updated as well as the 10 new vertices. With this change,
it is only necessary to update the 10 new vertices. That's 100X less work.
If getting data to populate vertices is slow, like when scaling data, the
need to update every vertex after calling allocate() can have an impact on
performance, i.e., dropped frames.

My review was rightfully rejected. I believe this is a valuable change but
am I the only one who will use this?

I have a second question. No reviewer commented on this so I don't know the
answer:

In a patch review, is it okay to touch documentation on functions that are
not modified?

Specifically, I want to change vertexCount()'s documentation
<https://doc.qt.io/qt-5/qsggeometry.html#vertexCount>to read: "Returns the
number of vertices that can be rendered, or if indices are used, it returns
the number of vertices that can be accessed through indices." The
description should change because it no longer returns the capacity of the
buffer -- the capacity may be much higher. Mentioning *indices* is
out-of-scope, but IMO sorely needed for this function. It may help others,
or me in 6 months if I finally find a use case for indexed drawing.

/Stan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20241030/9a47ceaa/attachment.htm>


More information about the Development mailing list