Culling of both cameras #1198

tomluchowski · 2016-06-03T12:47:58Z

As in title , now the code due to boost::geometry::convex_hull can cull both camera views at once .

hwoarangmy · 2016-06-06T07:37:21Z

@tomluchowski Could you please merge the commits so that there is only 1 functionality per commit ?
As I already said, splitting into several commits is a good habit but each commit should represent one functionality. For example, you could have one for culling in the main map and one in the minimap.
Then, if there are code changes requested, you can add commits to adapt (to avoid too much editing).

But with PRs like this one, history of the files will become a mess in only a few days and reading code to understand better what's going on when looking for some regression becomes not possible.
I will review the code when the commits are merged. In the meanwhile, I will test to see how it behaves.

tomluchowski · 2016-06-07T10:35:01Z

: naah by choice I appended to the akien's mga " Add changelog for 0.7.0"
Hopefully it's not a great blunder.

hwoarangmy · 2016-06-07T12:20:03Z

"Add changelog for 0.7.0" is not the best name I could think about for such a commit. Please rename it so the history is not broken. Keep in mind that if we look for file history, commits must be a minimum meaningfull. And in this case, it is missleading as I wouldn't expect such a commit to be culling code.

hwoarangmy · 2016-06-07T12:22:21Z

It seems also like your PR do not compile

akien-mga · 2016-06-07T12:25:36Z

Also I appear as author, and it contains my whole changelog additions, which are already in the development branch.

I would do:

git format-patch -1  // keep the 0001-*.patch file around
git remote add upstream https://github.com/OpenDungeons/OpenDungeons
git fetch upstream
git reset --hard upstream/development
// edit the patch to remove the part about RELEASE-NOTES.md
git apply 0001-*.patch  // reapply the patch, without committing, solve the conflict with RELEASE-NOTES.md
git status  // check that the changes are fine
git commit -a "Good name for a commit about culling"

tomluchowski · 2016-06-07T14:44:08Z

Well just look why travis is broken , it cannot upgrade it's repos :
The command "sudo apt-get update -qq; sudo apt-get install -y libcegui-0.8-dev" failed and exited with 100 during .

I will fix my commit to meaningful name when I am back home.

hwoarangmy · 2016-06-07T14:51:37Z

yes but appveyor complains at least of constexpr. Since we also support MSVS2013 and MinGW compilers, we must not use not supported c++11 features.

And since you are going to redo your commit, also note that you have uploaded a file EntityBase.cpp and .h that are not in use anymore.

tomluchowski · 2016-06-07T14:53:55Z

BTW : Could you and akien-mga relate to the issues I posted on our forum ?

tomluchowski · 2016-06-09T09:56:27Z

ok , hopefully I am done .

akien-mga · 2016-06-09T10:32:12Z

CMakeLists.txt

@@ -271,6 +271,9 @@ set(OD_SOURCEFILES

    ${SRC}/camera/CameraManager.cpp
    ${SRC}/camera/HermiteCatmullSpline.cpp
+    ${SRC}/camera/CullingManager.cpp
+    ${SRC}/camera/DummyArrayClass.cpp


This file does not exist, so the build fails:

CMake Error at CMakeLists.txt:652 (add_executable): Cannot find source file: /home/travis/build/OpenDungeons/OpenDungeons/source/camera/DummyArrayClass.cpp Tried extensions .c .C .c++ .cc .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp .hxx .in .txx

hwoarangmy · 2016-06-10T17:04:48Z

source/camera/CameraManager.h

@@ -62,7 +66,7 @@ class CameraManager
        fullStop
    };

-    CameraManager(Ogre::SceneManager* sceneManager, TileContainer* gameMap, Ogre::RenderWindow* renderWindow);
+    CameraManager(Ogre::SceneManager* sceneManager, GameMap* gameMap, Ogre::RenderWindow* renderWindow);


Why did you change TileContainer to GameMap ? TileContainer is not a very useful abstract level since it is almost not used but why did you change ?

That was a choice it seems./

ok, then it is a random change. Hopefully, GameMap is compatible with TileContainer. Seems like luck is on our side...

hwoarangmy · 2016-06-12T16:21:04Z

source/camera/SlopeWalk.cpp

+    rightVerticesIndex++;
+    mRightSlopeIndex++;
+    return rightVerticesIndex == rightVertices.end();
+


hwoarangmy · 2016-06-12T16:29:17Z

I've not signaled all the random empty lines because there were too many. Please at least remove when there are too many or when they hurt readability (like in a constructor init list).

hwoarangmy · 2016-06-12T19:51:54Z

I've had a quick look and seen that
SlopeWalk::mLeftVertexPassed
SlopeWalk::mRightVertexPassed

are set but not used. Is it some forgotten test variables ?

tomluchowski · 2016-06-12T20:38:05Z

@hwoarangmy : They are used in bool SlopeWalk::notifyOnMoveDown(int64_t newyIndex)
EDIT : but not too much . :P

hwoarangmy · 2016-06-13T08:07:22Z

source/camera/SlopeWalk.h

+
+    // index numbers in myArray of Vertices 
+    // belonging to the right path
+    std::deque<int32_t> rightVertices;


you really don't want to follow code guidelines...

What have I missed in code guidelines that I fail to implement in here ?

class member variables names must start with m. That is, you should rename this variable to mRightVertices.

hwoarangmy · 2016-06-13T08:17:28Z

@tomluchowski It can be done later but do you think it would be hard to split the CullingManager so that we can use one instance for the main screen and another for the minimap ?
That would decrease coupling between the CullingManager and the rest of the classes.

tomluchowski · 2016-06-13T09:33:28Z

That would be increadibly hard if not impossible. The whole algorithm needs to compute the convex_hull of ALL points exactly ONCE.

…hull from boost geometry used

tomluchowski · 2016-06-13T21:03:18Z

Could you also answer my post on the forum ?

hwoarangmy · 2016-06-13T21:20:43Z

source/camera/SlopeWalk.cpp

+    mVertices.mMyArray.clear();
+
+    boost::geometry::convex_hull(polygon, hull);
+    boost::geometry::for_each_point(boost::geometry::exterior_ring(hull), [this](Point &p){this->mVertices.mMyArray.push_back(p);});


For the next time, please split this into several lines to make clear there is a function here. Not blocking

hwoarangmy · 2016-06-13T21:24:33Z

I would have prefered to merge that after the release but at our current pace, that may be long. I have a PR almost done for allowing to change the minimap (switching from the old and the camera one) and if it gets merged before yours, you will have to make sure it works. I guess you would curse me for many generations if that was to happen ^^

Sorry for all the requested changes but the code looks much better now and is mergeable as for me

Could you also answer my post on the forum ?

Which one ?

tomluchowski force-pushed the development branch 2 times, most recently from 527fa61 to 9e34cd9 Compare June 7, 2016 10:31

tomluchowski force-pushed the development branch from 9e34cd9 to 7a3a06c Compare June 9, 2016 09:55

tomluchowski force-pushed the development branch from 7a3a06c to 0c57842 Compare June 9, 2016 10:07

akien-mga reviewed Jun 9, 2016
View reviewed changes

tomluchowski force-pushed the development branch 2 times, most recently from 9e99e62 to 7d5c1f8 Compare June 9, 2016 12:01

hwoarangmy reviewed Jun 10, 2016
View reviewed changes

hwoarangmy reviewed Jun 12, 2016
View reviewed changes

tomluchowski force-pushed the development branch 2 times, most recently from cb853c6 to a7e687f Compare June 12, 2016 21:14

hwoarangmy reviewed Jun 13, 2016
View reviewed changes

New Culling method now culling works for both of the cameras, convex_…

6297f7f

…hull from boost geometry used

tomluchowski force-pushed the development branch from a7e687f to 6297f7f Compare June 13, 2016 17:59

hwoarangmy reviewed Jun 13, 2016
View reviewed changes

hwoarangmy merged commit 40dec6d into OpenDungeons:development Jun 13, 2016

hwoarangmy mentioned this pull request Jun 14, 2016

Culling should not be processed when camera doesn't move #1215

Closed

hwoarangmy mentioned this pull request Aug 24, 2016

Do we want custom culling ? #924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Culling of both cameras #1198

Culling of both cameras #1198

tomluchowski commented Jun 3, 2016

hwoarangmy commented Jun 6, 2016 •

edited

tomluchowski commented Jun 7, 2016

hwoarangmy commented Jun 7, 2016

hwoarangmy commented Jun 7, 2016

akien-mga commented Jun 7, 2016

tomluchowski commented Jun 7, 2016

hwoarangmy commented Jun 7, 2016

tomluchowski commented Jun 7, 2016

tomluchowski commented Jun 9, 2016

akien-mga Jun 9, 2016

hwoarangmy Jun 10, 2016

tomluchowski Jun 11, 2016

hwoarangmy Jun 11, 2016 •

edited

hwoarangmy Jun 12, 2016

hwoarangmy commented Jun 12, 2016

hwoarangmy commented Jun 12, 2016

tomluchowski commented Jun 12, 2016 •

edited

hwoarangmy Jun 13, 2016

tomluchowski Jun 13, 2016

hwoarangmy Jun 13, 2016

hwoarangmy commented Jun 13, 2016 •

edited

tomluchowski commented Jun 13, 2016

tomluchowski commented Jun 13, 2016

hwoarangmy Jun 13, 2016

hwoarangmy commented Jun 13, 2016 •

edited

Culling of both cameras #1198

Culling of both cameras #1198

Conversation

tomluchowski commented Jun 3, 2016

hwoarangmy commented Jun 6, 2016 • edited

tomluchowski commented Jun 7, 2016

hwoarangmy commented Jun 7, 2016

hwoarangmy commented Jun 7, 2016

akien-mga commented Jun 7, 2016

tomluchowski commented Jun 7, 2016

hwoarangmy commented Jun 7, 2016

tomluchowski commented Jun 7, 2016

tomluchowski commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwoarangmy Jun 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwoarangmy commented Jun 12, 2016

hwoarangmy commented Jun 12, 2016

tomluchowski commented Jun 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwoarangmy commented Jun 13, 2016 • edited

tomluchowski commented Jun 13, 2016

tomluchowski commented Jun 13, 2016

Choose a reason for hiding this comment

hwoarangmy commented Jun 13, 2016 • edited

hwoarangmy commented Jun 6, 2016 •

edited

hwoarangmy Jun 11, 2016 •

edited

tomluchowski commented Jun 12, 2016 •

edited

hwoarangmy commented Jun 13, 2016 •

edited

hwoarangmy commented Jun 13, 2016 •

edited