Planeta again #293

Capital-Asterisk · 2024-06-17T01:09:11Z

Major terrain-related changes:

Add session to upload terrain mesh to GPU
Add temporary 'heightmap' function to terrain. (two cosine waves added together)
Increase number of terrain skeleton subdivision level from 10 to 24
Rejiggle terrain test scenario(s): Earth-sized planet for terrain scenario, and tiny planet for terrain_small scenario
A bunch of internal changes to planeta that are difficult to explain, to make the stuff above actually work.

Non-terrain-related changes:

Add src/osp/core/buffer_format.h, based on and improved from the "StrideDesc" nonsense from src/osp/universe/universe.h

… more

Conflicts: src/planet-a/chunk_generate.cpp

Capital-Asterisk · 2024-06-17T01:12:44Z

src/planet-a/chunk_utils.h

-    std::uint16_t m_edgeVrtxCount{0};
+
+    std::uint16_t m_fillVrtxCount{};
+    std::uint16_t m_edgeVrtxCount{};


Empty braces like this will zero-initialize (initialize to zero) to prevent an uninitialized value.

I don't have this in guidelines.md, but I'd intend that empty braces means "don't care what this value is, but don't keep it uninitialized," whereas {0} would imply that the zero is important.

Kind of a philosophical question here, but is there any actual difference between "Don't care what the value is, but don't leave it uninitialized" and "don't initialize" ?

Using empty-brace initialization for variables where you don't care what the resulting value is means that automated tools like clang-tidy / msvc-analyzer can't identify "read-from-uninitialized" bugs in the code, which i think is a big risk.

Note that I have no problem with the variables here being initialized to zero if that's what's intended. But "initialize them, but i don't care to what" is, in my opinion, worse than 'don't initialize'.

And, of course, this whole concept only applies to primative types. Class types / types with constructors implicitly manage their initial state.

Kind of a philosophical question here, but is there any actual difference between "Don't care what the value is, but don't leave it uninitialized" and "don't initialize" ?

If I look at code that looks like this:

int value; do_something(value);

value is uninitialized and there may be weird inconsistent errors when it's used wrong. static analysis may also scream at it. However, the intention is clear that the initial value of value is dont-care.

If I instead do...

int value = 0; do_something(value);

there's no risk of using uninitialized values, but the zero looks significant, as if it was a start of a count for example.

Instead using...

int value{}; do_something(value);

value would be zero-initialized, there's no risk of any weird inconsistent errors. The intention is default or don't-care, nothing is explicitly specified.

Using empty-brace initialization for variables where you don't care what the resulting value is means that automated tools like clang-tidy / msvc-analyzer can't identify "read-from-uninitialized" bugs in the code, which i think is a big risk.

Are you implying that uninitialized values can help find logic errors?

I believe we should avoid uninitialized values whenever possible. Using uninitialized memory as part of regular calculation logic something I'd mostly consider a bad idea nowadays.

PREfast also tends to scream at every uninitialized value.

I don't remember where I originally learned this from. I did find this recent similar discussion through google search results: https://www.reddit.com/r/cpp/comments/1dm2y2t/is_empty_brace_initialization_considered_best/

Since I'm noticing I'm throwing around the term "don't care" quite a bit, I would note that this is a term from digital logic; I'm not sure if it's used in other parts of computing.

Are you implying that uninitialized values can help find logic errors?

Not quite.

I'm saying that from the perspective of a static analyzer

int bob;

and

int alice{};

are very different looking.

If the programmer doesn't care about the value, then the programmer doesn't want the value to be read from until something is put there. A static analyzer (e.g. PREfast) can detect this kind of problem with bob, but alice looks initialized.

My advice is that empty brace initialization for primitive types is a bad idea exactly because of this, and explicitly setting a value is better.

I'd be interested to see an example where PREfast was saying "read from uninitialized!" when that wasn't happening. I suppose it could happen with non-const reference args to functions in other translation units, but that seems like a bug in PREfast that it isn't going and looking at the other translation unit.

Are you implying that uninitialized values can help find logic errors?

Not quite.

The further explanation makes this a yes? In your explanation, bob is kept uninitialized to use it to help find a potential logic error.

So from your point of view, would this example also be unacceptable?

int value = 0; // This can be anything, will be overwritten immediately do_something(value);

Since this is more about using uninitialized memory (less about zero-initialization), we're not the first to have this discussion:

https://softwareengineering.stackexchange.com/questions/305886/doesnt-always-initialize-variables-lead-to-important-bugs-being-hidden

I'm mostly on the side of just disallowing uninitialized memory. Undefined behaviour that does stuff like being different between debug vs release is far harder to debug IMO, compared to accidentally reading a zero.

Clang-tidy does have an option for this: cppcoreguidelines-init-variables

I'd be interested to see an example where PREfast was saying "read from uninitialized!" when that wasn't happening....

This alert is from this PR:

https://github.com/TheOpenSpaceProgram/osp-magnum/security/code-scanning/333

It doesn't even care about whether the variable is being read or not. It just doesn't like uninitialized members.

src/testapp/sessions/magnum.cpp

src/osp/core/buffer_format.h

jonesmz · 2024-06-23T12:07:34Z

src/osp/core/array_view.h

@@ -36,34 +38,52 @@ namespace osp
 using Corrade::Containers::ArrayView;
 using Corrade::Containers::arrayView;

+/**
+ * @brief Wraps a Corrade ArrayView or StridedArrayView to use as a 2D array of equally sized rows
+ */
 template<typename T>
 struct ArrayView2DWrapper


Since this class is only supposed to be used with ArrayView or StridedArrayView, you might want to consider adding template specializations for only those two, and leaving the "default" template declaration of the struct undefined.

The lazy way of just leaving it like this is also fine, of course.

Or a requires clause.

either way.

I guess you mean a concept for array views?

I'll be lazy on this as there's already a short comment explaining what this type is, and it's a little too simple.

jonesmz · 2024-06-23T12:09:37Z

src/osp/core/buffer_format.h

+    constexpr bool is_not_used() const noexcept { return stride == 0; }
+
+    std::size_t     offset{};
+    std::ptrdiff_t  stride{};


empty-brace-init instead of = 0 ?

I think I mentioned this in another review comment: my intention for an empty brace is "don't care what this value is, just not uninitialized." A zero explicitly indicates that the value is important to some business logic somewhere.

oh, it's this one: https://github.com/TheOpenSpaceProgram/osp-magnum/pull/293/files#r1642043900

jonesmz · 2024-06-23T12:11:22Z

src/osp/core/math_int64.h

 */
-constexpr bool is_distance_near(Vector3l const a, Vector3l const b, std::uint64_t const threshold) noexcept
+constexpr bool is_distance_near(Vector3l const a, Vector3l const b, double const threshold) noexcept


why switch to double?

The previous implementation here just doesn't work for the Earth-sized distances. I'd rather keep it simple and not write something ultra-optimized here.

jonesmz · 2024-06-23T12:12:40Z

src/osp/core/math_int64.h

-    std::uint64_t const dz = absdelta(a.z(), b.z());
+    double const dx( absdelta(a.x(), b.x()) );
+    double const dy( absdelta(a.y(), b.y()) );
+    double const dz( absdelta(a.z(), b.z()) );


Personally i don't find switching from assignment initialization to braced initialization easier to read.

Are you doing this to force an error if the type of the result of absdelta is not the same as the type of the variable?

I agree. Not particularly a sane move to do this on my end. Curly braces will enforce the type; however, these are those error-prone round braces that have a silly chance of being interpreted as a function declaration. I'm intentionally implicit-casting int64 from absdelta to a double.

jonesmz · 2024-06-23T12:18:14Z

src/osp/core/math_int64.h

-    std::uint64_t const magnitudeSqr = (dx*dx + dy*dy + dz*dz);
-
-    return magnitudeSqr < threshold*threshold;
+    return (dx*dx + dy*dy + dz*dz) < threshold*threshold;


You could check and see if the codegen is better or worse when using https://en.cppreference.com/w/cpp/numeric/math/hypot here.

I could def see an argument that using the sum of squares is faster than doing a square root, so i have no concerns about this code. Just pointing out that there's a special function for a different way to slice this.

src/planet-a/chunk_utils.cpp

jonesmz · 2024-06-23T12:24:32Z

src/planet-a/geometry.h

-    std::vector<osp::Vector3>           chunkVbufPos;
-    std::vector<osp::Vector3>           chunkVbufNrm;
-    std::vector<osp::Vector3u>          chunkIbuf;
+    Corrade::Containers::Array<unsigned char> vrtxBuffer; ///< Output vertex buffer


unsigned char, specifically?

not std::byte, or just char?

jonesmz · 2024-06-23T12:25:25Z

src/planet-a/icosahedron.h

 {
-    1.05146222e+0f,  5.46533058e-1f,  2.75904484e-1f,  1.38283174e-1f,  6.91829904e-2f,
-    3.45966718e-2f,  1.72989830e-2f,  8.64957239e-3f,  4.32479631e-3f,  2.16239942e-3f
+    1.05146222e+0f,  5.46533058e-1f,  2.75904484e-1f,  1.38283174e-1f,


could you document where the initial values of this are from? Is it icosahedron_tables.py ?

There's already comment; it's just not visible on the diff.

jonesmz · 2024-06-23T12:25:50Z

src/planet-a/icosahedron.h

-    4.13372560e-2f,  2.06730441e-2f,  1.03370743e-2f,  5.16860619e-3f,  2.58431173e-3f
-};
+inline constexpr std::array<float, 24> const gc_icoMaxEdgeVsLevel
+{{


double brace init here? that seems like maybe a typo?

I don't remember the exact details, but this is required for some of the other tables in this file for arrays of structs. This is harmless afaik

src/testapp/scenarios.cpp

Capital-Asterisk added 6 commits May 28, 2024 01:28

Implement planeta chunk mesh offset

c79dff7

Add buffer_format.h, based on src/osp/universe.h

35965e5

Add planeta terrain mesh origin offset, Magnum GL mesh rendering, and…

680d0aa

… more

Clean up terrain code

20d8c87

Merge branch 'planeta-again'

41c216f

Conflicts: src/planet-a/chunk_generate.cpp

Update terrain.cpp: remove temporary code, add comment

4ef82be

Capital-Asterisk commented Jun 17, 2024

View reviewed changes

github-advanced-security bot found potential problems Jun 17, 2024

View reviewed changes

src/testapp/sessions/magnum.cpp Fixed Show resolved Hide resolved

Lamakaio approved these changes Jun 17, 2024

View reviewed changes

src/osp/core/buffer_format.h Outdated Show resolved Hide resolved

Fix stuff according to PR TheOpenSpaceProgram#293

b4141b7

Lamakaio approved these changes Jun 20, 2024

View reviewed changes

jonesmz reviewed Jun 23, 2024

View reviewed changes

src/planet-a/chunk_utils.cpp Outdated Show resolved Hide resolved

jonesmz reviewed Jun 23, 2024

View reviewed changes

src/testapp/scenarios.cpp Outdated Show resolved Hide resolved

jonesmz approved these changes Jun 23, 2024

View reviewed changes

Make more changes according to PR#293, Fix clang-tidy warnings

dd7db8b

Capital-Asterisk merged commit b6bd827 into TheOpenSpaceProgram:master Jul 2, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Planeta again #293

Planeta again #293

Capital-Asterisk commented Jun 17, 2024

Capital-Asterisk Jun 17, 2024

jonesmz Jun 23, 2024

jonesmz Jun 23, 2024

jonesmz Jun 23, 2024

Capital-Asterisk Jun 25, 2024

Capital-Asterisk Jun 25, 2024

jonesmz Jun 26, 2024

Capital-Asterisk Jun 27, 2024

jonesmz Jun 23, 2024

jonesmz Jun 23, 2024

Capital-Asterisk Jun 25, 2024

jonesmz Jun 23, 2024

Capital-Asterisk Jun 25, 2024

Capital-Asterisk Jun 25, 2024

jonesmz Jun 23, 2024

Capital-Asterisk Jun 25, 2024

jonesmz Jun 23, 2024

Capital-Asterisk Jun 25, 2024

jonesmz Jun 23, 2024

jonesmz Jun 23, 2024

jonesmz Jun 23, 2024

jonesmz Jun 23, 2024

Capital-Asterisk Jul 2, 2024

jonesmz Jun 23, 2024

Capital-Asterisk Jul 2, 2024

Planeta again #293

Planeta again #293

Conversation

Capital-Asterisk commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment