Lambdas in CreateRandomHeightMap function #1
Conversation
jantosi
left a comment
jantosi
left a comment
There was a problem hiding this comment.
I like how you wanted to create some atomic functions and then compose them into larger operations. Please work a bit more on expressing your intent with naming and conventions so that another developer could pick these atoms and create a new composition with ease.
EngineDemo/terrain.cpp
Outdated
|
|
||
| auto get = [&heightmap, sizeX = mInfo.HeightmapWidth, sizeY = mInfo.HeightmapHeight](int x, int y) -> decltype(mHeightmap)::value_type | ||
| { | ||
| if (x < 0 || x >= sizeX || y < 0 || y >= sizeY) |
There was a problem hiding this comment.
Perfect use-case of ? : operator
EngineDemo/terrain.cpp
Outdated
|
|
||
| auto& heightmap = mHeightmap; | ||
|
|
||
| auto get = [&heightmap, sizeX = mInfo.HeightmapWidth, sizeY = mInfo.HeightmapHeight](int x, int y) -> decltype(mHeightmap)::value_type |
There was a problem hiding this comment.
All of these auto variablename = ... lambdas could be refactored into declared methods on some util class. You don't need lambdas here, since these aren't brief, aren't used ad hoc and finally, variables holding these should be constants instead.
I can see the value of learning how to write proper lambdas. I would, however, write normal methods here.
| auto average = [](std::initializer_list<float> list) | ||
| { | ||
| float sum = std::accumulate(std::cbegin(list), std::cend(list), 0.0, | ||
| [](auto&& a, auto&& b) { |
There was a problem hiding this comment.
This is a perfect lambda: small, inline, extraction into a method would decrease simplicity. I would go even further and make it a one-liner
EngineDemo/terrain.cpp
Outdated
| return a + (b != -1 ? b : 0); | ||
| }); | ||
|
|
||
| int total = std::count_if(std::cbegin(list), std::cend(list), [](auto&& val) { return val != -1; }); |
There was a problem hiding this comment.
Filtering! Great use of lambdas 👍
EngineDemo/terrain.cpp
Outdated
| std::mt19937 gen(rd()); | ||
| std::uniform_real_distribution<> dis(-1, 1); | ||
|
|
||
| auto random = [&dis, &gen]() { return dis(gen); }; |
There was a problem hiding this comment.
This looks familiar, function composition? I would extract composeFunctions(fn1, fn2)
EngineDemo/terrain.cpp
Outdated
|
|
||
| auto random = [&dis, &gen]() { return dis(gen); }; | ||
|
|
||
| std::function<void(int)> divide = [÷, &random, &square, &diamond, sizeX = mInfo.HeightmapWidth - 1, sizeY = mInfo.HeightmapHeight - 1](int size) |
There was a problem hiding this comment.
I realise why capture lists are needed in C++, but this makes the perfect case against them ;) Sooo long
Also, naming of this lambda is quite confusing; dividing what into what?
EngineDemo/terrain.cpp
Outdated
| divide(half); | ||
| }; | ||
|
|
||
| set(0, 0, 0.0); |
There was a problem hiding this comment.
Are these following lines a sort of unit-test?
| { | ||
| mImmediateContext->PSSetShaderResources(2, 1, &mLayerMapArraySRV); | ||
| return; | ||
| //return; |
There was a problem hiding this comment.
Committing commented-out code is a no-no
EngineDemo/terrain.cpp
Outdated
| { | ||
| float sum = std::accumulate(std::cbegin(list), std::cend(list), 0.0, | ||
| [](auto&& a, auto&& b) { | ||
| return a + (b != -1 ? b : 0); |
There was a problem hiding this comment.
Is this supposed to assume value 0 of -1-elements? If so, this needs to be expressed somewhere. Right now this looks like magic and could introduce hard-to-debug logic.
EngineDemo/terrain.cpp
Outdated
| return heightmap[y*sizeX + x]; | ||
| }; | ||
|
|
||
| auto set = [&heightmap, sizeX = mInfo.HeightmapWidth, sizeY = mInfo.HeightmapHeight](int x, int y, auto val) -> void |
There was a problem hiding this comment.
You're not using sizeY from capture list anywhere in this lambda
- update VisualCppTools
- deprecate LoadShader function
One function of a larger object
We can assume that defined lambdas will not be necessary anywhere else in any foreseeable future
The main question is: is it ok, or should I declare proper methods and such?
Sam widzę kilka problemów z łapaniem lokalnych zmiennych, ale to jeszcze powinienem być wstanie ogarnąć lepiej