Skip to content
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

[Review phase] Terrain texture selection #1769

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@unelsson
Copy link
Contributor

unelsson commented Jun 18, 2018

Terrain texture selection, based on #1414

Feedback and review is greatly appreciated. Plutonicoverkill's code had some reviews, but I don't clearly understand what are the reasonings behind those comments.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 19, 2018

And here we reach the point where not having scrawl around really hurts, because I am not familiar with this stuff either.

Anyway, I did a test run and ended up with a segfault on attempting to select terrain: https://gist.github.com/zinnschlag/0916aa134ae01a2484ed24f7f5faef9d

@psi29a psi29a changed the title [Don't merge yet] Terrain texture selection [WIP] Terrain texture selection Jun 19, 2018

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jun 19, 2018

Get in the habit of using WIP in stead of Don't Merge :)

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jun 19, 2018

I know that @kcat has some experience with terrain... perhaps he can help review?

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Jun 19, 2018

I haven't got any segfaults, was there anything specific you were doing, or just clicking primary select? I don't really know how to read that gistfile, but looks like activate-function doing something with Qt.

I really don't know... Version mismatch? I'm using Qt version 5.11.0

I'll try to click everywhere and see if I can get a segfault somehow. (edit: I can't find a way to crash it)

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 19, 2018

Just random clicking. Still on Qt 4, but from the stack backtrace it is pretty clear that this is not a Qt problem.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 19, 2018

#0 0x00000000008a7451 in ESM::Land::loadData (this=this@entry=0x7fffffffb1c0, flags=flags@entry=2,
target=target@entry=0x0) at /home/marc/OpenMW/openmw/components/esm/loadland.cpp:220
#1 0x00000000008a7c62 in ESM::Land::getLandData (this=0x7fffffffb1c0, flags=flags@entry=2)
at /home/marc/OpenMW/openmw/components/esm/loadland.cpp:349

This looks highly suspicious. Just saying.

@kcat

This comment has been minimized.

Copy link
Contributor

kcat commented Jun 19, 2018

I know that @kcat has some experience with terrain... perhaps he can help review?

I'm not particularly familiar with terrain-related stuff. Especially on the editor side of things.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jun 20, 2018

@zinnschlag what file(s) are you loading to trigger this? That stacktrace looks like something in the loadland code.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 20, 2018

Morrowind.esm. I doubt that it is the load code itself. Notice the this value? Very unusual. Could be a multi-threading problem or maybe an object life-time problem. Only guessing here.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jun 20, 2018

Qt4 uses threaded renderer whilet Qt5 uses single-threaded renderer... not sure if that makes a difference?

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 20, 2018

Not really. I was speaking about threading in our terrain system.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 22, 2018

Apparently nobody (including me) noticed the elephant in the room: Why is it trying to load terrain when I select a piece of terrain? Obviously the terrain was already loaded. Otherwise it could not have been rendered which means I couldn't have clicked on it to select it.

@Aesylwinn

This comment has been minimized.

Copy link
Contributor

Aesylwinn commented Jun 23, 2018

Could be your click was interpreted as a being in one of the unloaded cells. Maybe try moving away from loaded cells and try reproducing it. I think the worldpos returned defaults to a set distance when nothing gets clicked on. (Unloaded cells just aren't rendered iirc).

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 23, 2018

Lol. I compiled your code again and then noticed the warning:

/home/marc/OpenMW/openmw/apps/opencs/view/render/terrainselection.cpp:17:67: warning: a temporary bound to ‘CSVRender::TerrainSelection::mCoords’ only persists until the constructor exits [-Wextra]
:mCoords {coords}, mEsmLand {esmLand}, mParentNode {parentNode}

So, yeah. One of my guesses turned out to be right. Object life-time issue.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Jun 26, 2018

I'm trying to reproduce or understand the issue, but I don't get any errors, warnings or crashes, and furthermore, I'm clueless why mCoords would persist only until the constructor exits, as to my understanding it's defined in terrainselection.hpp correctly. If this is clear, I'd be really grateful if someone could explain this to me.

I've stopped development until this is resolved. I don't know how to approach this problem, as Zini's comments are only info I have of this, and I can't understand a problem in the code. There are some other glitches too, but crashing is so fundamental that it's probably better fixed first. There's a noticeable offset from selection grid to actual world texture grid, and cell borders don't operate/render correctly. Drag-select is also still missing.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jun 26, 2018

What platform are you guys on? Windows, Linux or MacOS? Could be a difference in compilers... msvc vs gcc vs clang?

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Jun 26, 2018

Linux *** 4.9.107-2-MANJARO #1 SMP PREEMPT x86_64 GNU/Linux

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 27, 2018

That you don't get the crash isn't surprising, because undefined behaviour. However that you don't get a warning message is surprising.

Anyway, an easy fix would be to change mCoors from const CSMWorld::CellCoordinates& to simply CellCoordinates. Reference as member variables are rather strange to begin with. Not outright wrong, but most of the time a bad idea. Still trying to track down the source of the problem would be a good idea.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 27, 2018

I'm clueless why mCoords would persist only until the constructor exits

Just to make this clear: It is not mCoors (because mCoors is a reference). The compiler is complaining about a temporary that is bound to mCoors.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Jun 27, 2018

To be sure, I compiled it again from the start, and searched the log for that warning, and keywords (terrainselect.cpp) etc. but found no warnings related to this. I don't really understand when using references or pointers is good/bad, but I didn't change anything that didn't work.

Thanks for clarification anyway, it gives me at least one place to look.

@Aesylwinn

This comment has been minimized.

Copy link
Contributor

Aesylwinn commented Jun 28, 2018

Compiler bug: https://stackoverflow.com/questions/10509603/why-cant-i-initialize-a-reference-in-an-initializer-list-with-uniform-initializ

I'm guessing Zini is using gcc 4. Use regular initialization to work around it.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented Jun 28, 2018

@zinnschlag if you're toolchain is buggy, time to upgrade ;)

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 28, 2018

4.8.4. And not upgrading yet (too much stuff that would break and absolutely no time to deal with that).

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Jun 28, 2018

The code is not written by me, and I'm still fairly beginner in C++, so I'm going to milk some knowledge out of you to fix this, and learn.

So, am I understanding this correctly? Because of C++ compiler bug/feature, there's some undefined behavior occurring from list-initialization in format: variable {parameter}

and that would be fixed by using "default initialization" in format: variable (parameter)?

The former should work in C++11, but not necessarily in C++ versions before that? Does this mean that list-initialization cannot be used at all in OpenMW?

In addition, I don't see any advantages for using initializer-list with only one parameter. Is this true?

And fundamentally, I'm understanding that initializer list (member initializer list) is a different thing than list-initialization? Former is the thing happening after : in constructor, and latter is something like variable {}.

To the problem at hand, the problem seems to be affecting mCoords, as it's initialized with a parameter that is a reference. In this case, CSMWorld::CellCoordinates& coords instead of CSMWorld::CellCoordinates coords? I also don't see any advantages in using a reference here.

The full initialization list where the problem occurs also initializes variables mEsmLand {esmLand} and mParentNode {parentNode}, of which mEsmLand is a reference. Shouldn't that be changed also?

Do I need to change both - the list-initialization to default initialization, and references to default variables?

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 28, 2018

So, am I understanding this correctly? Because of C++ compiler bug/feature, there's some undefined behavior occurring from list-initialization in format: variable {parameter}

and that would be fixed by using "default initialization" in format: variable (parameter)?

I am not entirely sure if that is what is happening here (since the error message is noticeable different from what I am getting). But it is worth a try.

The former should work in C++11, but not necessarily in C++ versions before that

The former is a C++11 feature. It did not exist before C++11.

In addition, I don't see any advantages for using initializer-list with only one parameter. Is this true?

The initiliser-list is nearly always preferable over initilaising in the function body. Its cleaner, for some types it avoids redundant initialisation and for other types it is mandatory.

To the problem at hand, the problem seems to be affecting mCoords, as it's initialized with a parameter that is a reference. In this case, CSMWorld::CellCoordinates& coords instead of CSMWorld::CellCoordinates coords? I also don't see any advantages in using a reference here.

The problem is not the parameter. The problem is the variable itself. mCoors should not be a reference.

Let me reiterate that: Having a reference as a class member variable is usually a bad idea. It is not outright wrong (there are certainly situation where I have used them in the past), but there is typically little benefit to it and a couple of potential disadvantages (objects of the class become un-assignable for example). I would also argue that it is somewhat counter-idiomatic.

The full initialization list where the problem occurs also initializes variables mEsmLand {esmLand} and mParentNode {parentNode}, of which mEsmLand is a reference. Shouldn't that be changed also?

You can not simply remove the reference, because that would make a copy and that would be wrong in this case. However using a pointer instead of a reference would express the intend more clearly.

C++ does not have the concept of value and reference types (as in languages like C#). But CellCoordinates comes pretty close to a value type, while ESM::Land (in this context) is pretty close to a reference type.

@Aesylwinn

This comment has been minimized.

Copy link
Contributor

Aesylwinn commented Jun 28, 2018

Initializer list is an ambiguous term. https://en.cppreference.com/w/cpp/utility/initializer_list

Using {} over () is purely personal preference unless using initializing a vector, list, etc. with data. Here the c++ 11 standard was amended and gcc4 implements the original behavior.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Jun 28, 2018

I ran into error message "no match for 'operator*" when backtracking and changing the reference of mCoords to a pointer, in mcoordinates in cell.cpp line 147. ( https://github.com/unelsson/openmw/blob/5419f4427b66302c2a3f37be751981804fd33b77/apps/opencs/view/render/cell.cpp#L147 ). There's still a reference as a member variable...

Anyway, changed the {} to () and tested, on my computer it works without crashing as it did before.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Jun 28, 2018

mCoords to a pointer

Nope. Not mCoors. As mentioned above that is as close as possible to a value type as something in C++ can be. Store it by value and pass it by reference.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Jun 30, 2018

While digging into what's a pointer and what's a reference and what to do, I encountered another thing I didn't understand: What would be the point in auto and static_cast at terrainselection.ccp lines 59-60 & 67-68?

    const auto x = static_cast<int>(std::floor(toCellCoords(worldPos).first));
    const auto y = static_cast<int>(std::floor(toCellCoords(worldPos).second));

Couldn't that be as well just normal conversion from double to int?

    const int x = (int)(toCellCoords(worldPos).first + 0.5);
    const int y = (int)(toCellCoords(worldPos).second + 0.5);
@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Feb 7, 2019

This PR is far ahead of #1414, so imho that should be closed. I left out the switch terrainselectiontype, which is still in #1414, but it isn't functional, and implementing that should be easy whenever terrain shape/vertice edit is developed. I didn't work a whole lot on terrain vertice selection, but I think this code is a good base for that later.

I'll add changelog and group those few lines.... I hope it won't trigger another builds though.

About squashing commits, I can't remember if there are commits that wont build. All have worked on my end. Should I just squash them all? Never done that though, so I'd better be careful not to mess up my whole git.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Feb 9, 2019

TODO:

  • The shape of the area affected by the selection change is defined by the state of the brush button.
  • Modify changelog.
  • Fix onlyadd selection mechanism to proper toggle
  • Fix maths: vertexcoords should use (landshape - 1), texturecoords to worldcoord conversion is required as int, but might work as std::pair, add vertexcoords to worldcoords

@unelsson unelsson changed the title [Review please] Terrain texture selection [Outdated] Terrain texture selection Apr 4, 2019

@unelsson unelsson force-pushed the unelsson:terraintextureselection branch from d961c51 to c36a968 Apr 7, 2019

@unelsson unelsson changed the title [Outdated] Terrain texture selection [Review phase] Terrain texture selection Apr 7, 2019

{
for(auto const& localPos: localPositions)
{
auto iter = std::find(mSelection.begin(), mSelection.end(), localPos);

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

iter is never used if itertemp is found, and std::find complexity is relatively high. You may want to put it into the if outer body.

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

Good point!

vertices->push_back(osg::Vec3f(CSMWorld::CellCoordinates::textureSelectionToWorldCoords(x) + nudgeOffset, drawPreviousY - nudgeOffset, calculateLandHeight(x1, y1+(i-1))+2));
vertices->push_back(osg::Vec3f(CSMWorld::CellCoordinates::textureSelectionToWorldCoords(x) + nudgeOffset, drawCurrentY - nudgeOffset, calculateLandHeight(x1, y1+i)+2));
}
}

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

Is there really no hope for code duplication reduction here? :(

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

I haven't found a way yet. Any ideas?

{
for(int j = texCoords.second - r; j <= texCoords.second + r; ++j)
{
selections.emplace_back(std::make_pair(i, j));

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

You don't need to use std::make_pair with emplace_back here and after.

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

I want to put a std::pair, holding values of i and j, in a vector. Is there another option? Fixed.

int distanceX = abs(i - texCoords.first);
int distanceY = abs(j - texCoords.second);
int distance = std::round(sqrt(pow(distanceX, 2)+pow(distanceY, 2)));
if (distance < r) selections.emplace_back(std::make_pair(i, j));

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

I noticed of course that this syntax is used elsewhere, but I wonder if these loops could be rewritten like

        for (int i = -r; i <= r; i++)
        {
            for (int j = -r; j <= r; j++)
            {
                osg::Vec2f coords(i,j);
                if (std::round(coords.length()) < r)
                    selections.emplace_back(i + texCoords.first, j + texCoords.second);
            }
        }

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

Probably they could. That looks okay. I wrote it on multiple lines in this syntax to cut the process into parts, to make formulas easier to understand, but I don't think your proposition is unclear. Umm.. Is this the way how pairs can be made with emplace_back?

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

Yes.

{
newId = CSMWorld::LandTexture::createUniqueRecordId(0, counter);
freeIndexFound = true;
}
} while (freeIndexFound == false);
}
while (freeIndexFound == false);

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

You probably shouldn't touch the formatting of this function, it's not that outrageous.

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

Yeh. Not very significant maybe, but changed it back.

@@ -91,7 +108,7 @@ void CSVRender::TerrainTextureMode::primaryEditPressed(const WorldspaceHitResult
CSMWorld::IdCollection<CSMWorld::LandTexture>& landtexturesCollection = document.getData().getLandTextures();
int index = landtexturesCollection.searchId(mBrushTexture);

if (index != -1 && !landtexturesCollection.getRecord(index).isDeleted() && hit.hit == true)
if (index != -1 && !landtexturesCollection.getRecord(index).isDeleted() && hit.hit && hit.tag == 0)

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

Extra space.

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

Precise reviewing. Thanks, will fix.

}

void CSVRender::TerrainTextureMode::setBrushTexture(std::string brushTexture)
{
mBrushTexture = brushTexture;
}

CSVRender::PagedWorldspaceWidget& CSVRender::TerrainTextureMode::getPagedWorldspaceWidget()

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

This function doesn't seem to be used anymore.

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

True that. Removed.

{
differentialPos.first = value.first - selectionCenterX;
differentialPos.second = value.second - selectionCenterY;
mCustomBrushShape.push_back(differentialPos);

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

Maybe use something like

        mCustomBrushShape.clear();
        for (auto const& value: terrainSelection)
            mCustomBrushShape.emplace_back(value.first - selectionCenterX, value.second - selectionCenterY);

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

That's nice. Does const& ref here avoid making a copy of the variable?

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

Well, yes, since it's a reference to the element.

namespace CSVWidget
{
class SceneToolTextureBrush;
}

namespace CSVRender
{
class PagedWorldspaceWidget;

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

This forward declaration is no longer necessary.

@@ -18,22 +19,34 @@
#include "../../model/world/landtexture.hpp"
#endif

#include "terrainselection.hpp"

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

I think it should be possible to forward-declare TerrainSelection instead of including this header.

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

I tried this, then added missing include osg/Group, but this still results in some hefty compile errors. "/usr/include/c++/8.2.1/bits/unique_ptr.h:79:16: error: invalid application of ‘sizeof’ to incomplete type ‘CSVRender::TerrainSelection’" Doesn't ring any obvious bells how to fix this.

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

OK, never mind, it can't be done cleanly.


void CSVRender::TerrainSelection::activate()
{
mParentNode->addChild(mSelectionNode);

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

So as scrawl said you should add a if (!mParentNode->containsNode(mSelectionNode)) check here.

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

Right.. And if (mParentNode->containsNode(mSelectionNode)) for deactivate/removeChild?

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

No, removing non-existent child is safe, i.e. no-op.

@@ -6,6 +6,8 @@
#include <string>
#include <memory>

#include <osg/Group>

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

Forward-declare it.

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

Ok.

void CSVRender::TerrainSelection::addSelect(const std::pair<int, int> localPos)
{
if (std::find(mSelection.begin(), mSelection.end(), localPos) == mSelection.end())
mSelection.emplace_back(localPos);

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

Four extra spaces here.

Is update call necessary if the selection wasn't changed?

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

I'll put update and emplace_back inside {} to prevent unnecessary update calls.

@@ -66,6 +79,9 @@ namespace CSVRender
/// \brief Handle brush mechanics, maths regarding worldspace hit etc.
void editTerrainTextureGrid (const WorldspaceHitResult& hit);

/// \brief Handle brush mechanics for texture selection
void selectTerrainTextures (std::pair<int, int>, unsigned char, bool);

This comment has been minimized.

Copy link
@Capostrophic

Capostrophic Apr 7, 2019

Collaborator

Since the coords pair isn't modified, it should be passed by reference, not value. The arguments should be named here for clarity of their purpose too.

This comment has been minimized.

Copy link
@unelsson

unelsson Apr 7, 2019

Author Contributor

True, I'll fix this.

@Capostrophic

This comment has been minimized.

Copy link
Collaborator

Capostrophic commented Apr 7, 2019

When I use secondary select, this happens when the selections overlap:
изображение
Pretty odd. What is it supposed to do?

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Apr 7, 2019

When I use secondary select, this happens when the selections overlap:
изображение
Pretty odd. What is it supposed to do?

I tried several versions of this during development. Now it toggles selected to unselected, and unselected to selected. This is the best toggle-type functionality I could think of.

unelsson added some commits Apr 7, 2019

@AnyOldName3

This comment has been minimized.

Copy link
Member

AnyOldName3 commented Apr 8, 2019

Might it be sensible to have a button to toggle between AND, OR and XOR instead of hardcoding in XOR?

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Apr 8, 2019

You mean AND/OR/XOR how secondary edit works? Extra button adds UI complexity, so I'm leaning towards hardcoding one functionality, though I also thought of possibility to do on/off toggle based on status of the center of click. Either way, doesn't seem like the best UI imho, but I understood Zini was planning some sort of toggle.

I'd personally see it handy to have only add-to-selection and remove-from-selection, maybe tied straight to primary and secondary selection. Anyhow, if consensus or clear plan is lacking, this can also be changed later, it's somewhat easy to change.

@unelsson

This comment has been minimized.

Copy link
Contributor Author

unelsson commented Apr 10, 2019

Almost forgot - should texture selection select textures only from loaded cells? Now it selects everything, regardless if cell is loaded or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.