Replace PriorityQueue class with it's stl equivalent #218

GGiter · 2019-11-14T23:35:28Z

No description provided.

JDuchniewicz · 2019-11-15T07:27:25Z

PolyEngine/Engine/Src/AI/PathfindingSystem.cpp


 			AllNodes.push_back(s);
-			openList.Push(std::make_pair(AllNodes.size() - 1, s.TotalCost()));
+			openList.push(std::make_pair(AllNodes.size() - 1, s.TotalCost()));


Consider constructing element in-place with emplace, this applies to whole PR

MuniuDev

Nice job. Consider applying @JDuchniewicz comment. LGTM

MuniuDev · 2019-11-19T06:37:44Z

PolyEngine/Engine/Src/Resources/MeshResource.cpp

-		std::vector<::pe::core::storage::PriorityQueue<std::pair<u8, float>, std::function<bool(const std::pair<u8, float>&, const std::pair<u8, float>&)>>> tmpBonesList;
-		tmpBonesList.resize(mesh->mNumVertices, { [](const std::pair<u8, float>& v1, const std::pair<u8, float>& v2) { return v1.second > v2.second; } });
+
+		std::vector<std::priority_queue<std::pair<u8, float>,std::vector<std::pair<u8, float>>, std::function<bool(const std::pair<u8, float>&, const std::pair<u8, float>&)>>> tmpBonesList;


Wow, that's a mouthful. Nice job though.

how about usings for this one?

That's actually a good idea. Let's do it.

MuniuDev · 2019-11-19T21:49:24Z

One comment, emplace create object in place in the container. Thus when changing it from push to emplace replace this:
q.push(MyObj(arg1, arg2));
with this:
q.emplace(arg1, arg2);

MuniuDev · 2019-11-19T21:53:14Z

PolyEngine/RenderingDevice/OpenGL/Src/GLWorldRendering.cpp

 			if (meshCmp->GetBlendingMode() == eBlendingMode::OPAUQE)
 			{
-				sceneView.OpaqueQueue.push(meshCmp);
+				sceneView.OpaqueQueue.emplace(meshCmp);


When you're not constructing object inside or right before push, don't bother with emplace.
It's actually a decline in performance.

MuniuDev · 2019-11-19T21:54:03Z

PolyEngine/Engine/Src/Resources/MeshResource.cpp

 					size_t vertId = vertWeight.mVertexId;
 					float weight = vertWeight.mWeight;
-					tmpBonesList[vertId].push({ boneId, weight });
+					tmpBonesList[vertId].emplace(std::pair<u8, float>( boneId, weight ));


Using emplace in such cases is good, just don't call the constructor, emplace will call it for you. Just provide it with constructor's arguments.

GGiter · 2019-11-20T17:09:47Z

PolyEngine/Core/Src/pe/core/storage/impl/IndexedStringManager.cpp

+{
+	if(m_ttlEnabled && !enabled)
+	{
+		bool bResult = m_ttlEntries.empty();


not sure what to do here i keep getting "discarding return value of function with 'nodiscard' attribute queue.empty()" warning which is then treated as error by compiler

MuniuDev · 2019-12-01T12:13:33Z

PolyEngine/Core/Src/pe/core/storage/impl/IndexedStringManager.cpp

 	if(m_ttlEnabled && !enabled)
 	{
-		m_ttlEntries.Clear();
+		DISCARD m_ttlEntries.empty();


This is not clearing the queue. Replace with m_ttlEntries.c.clear()

MuniuDev · 2019-12-01T12:14:51Z

PolyEngine/Engine/Src/AI/PathfindingSystem.cpp


 	AllNodes.push_back(PathNode(startNode, 0, graph->GetHeuristicCost(startNode, destNode) ));
-	openList.Push(std::make_pair(AllNodes.size() - 1, graph->GetHeuristicCost(startNode, destNode)));
+	openList.emplace(std::make_pair(AllNodes.size() - 1, graph->GetHeuristicCost(startNode, destNode)));


No need for make_pair here, since we have emplace.

MuniuDev · 2019-12-01T12:15:09Z

PolyEngine/Engine/Src/AI/PathfindingSystem.cpp


 			AllNodes.push_back(s);
-			openList.Push(std::make_pair(AllNodes.size() - 1, s.TotalCost()));
+			openList.emplace(std::make_pair(AllNodes.size() - 1, s.TotalCost()));


No need for make_pair here, since we have emplace.

MuniuDev · 2019-12-01T12:15:15Z

PolyEngine/Engine/Src/AI/PathfindingSystem.cpp

 		}

-		closedList.Push(std::make_pair(qIdx, AllNodes[qIdx].TotalCost()));
+		closedList.emplace(std::make_pair(qIdx, AllNodes[qIdx].TotalCost()));


No need for make_pair here, since we have emplace.

JDuchniewicz · 2019-12-07T13:26:07Z

PolyEngine/Core/Src/pe/core/storage/impl/IndexedStringManager.cpp

 	if(m_ttlEnabled && !enabled)
 	{
-		DISCARD m_ttlEntries.empty();
+		std::swap(m_ttlEntries, std::priority_queue<TTLEntry>());


This does not compile because you are trying to swap into a temporary (rvalue) and unless it is called as member function as shown here it won't.
Why just not call clear on the m_ttlEntries?

I believe stl priority queue doesn't have clear method.

…to dev

JDuchniewicz · 2020-01-28T19:30:52Z

PolyEngine/Tests/CoreTests/Src/PriorityQueueTests.cpp

+		CHECK(q.size() == i+1);
+	}
+
+	for (int i = testSize-1; i >= 0; --i)


Do we have any preference for spacings in such cases?

JDuchniewicz · 2020-01-28T19:31:09Z

PolyEngine/Tests/CoreTests/Src/PriorityQueueTests.cpp

+	}
+}
+
+TEST_CASE("PriorityQueue reverrse sorted push test", "[PriorityQueue]")


JDuchniewicz · 2020-01-28T19:33:13Z

PolyEngine/Core/Src/pe/core/storage/PriorityQueue.hpp

+
+namespace pe::core::storage
+{
+	template<class _Ty,class _Pr = std::less<typename std::vector<_Ty>::value_type> >


I would stick to normal engine conventions for template parameters, underscores are better left off in the stl implementation

JDuchniewicz · 2020-01-28T19:33:34Z

PolyEngine/Core/Src/pe/core/storage/impl/IndexedStringManager.cpp

@@ -1,5 +1,6 @@
 #include <pe/core/storage/impl/IndexedStringManager.hpp>

+


unnecessary newline

GGiter added 2 commits November 15, 2019 00:27

replace PriorityQueue class with stl priority queue

e243e03

delete old PriorityQueue class and it's tests

59ebd68

JDuchniewicz reviewed Nov 15, 2019

View reviewed changes

MuniuDev approved these changes Nov 19, 2019

View reviewed changes

GGiter added 2 commits November 19, 2019 21:58

replace priority_queue push with emplace

1a3a45d

add using keyword for long declarations

33a1eeb

MuniuDev requested changes Nov 19, 2019

View reviewed changes

GGiter added 2 commits November 20, 2019 17:45

replace emplace with push when not constructing element in place

453fb86

fix merge conflicts

c1e7c1a

GGiter commented Nov 20, 2019

View reviewed changes

replace unneeded assignments with DISCARD

216cfdd

GGiter requested a review from MuniuDev November 30, 2019 23:00

MuniuDev requested changes Dec 1, 2019

View reviewed changes

GGiter force-pushed the dev branch from 1e306ad to 216cfdd Compare December 3, 2019 18:42

GGiter added 2 commits December 3, 2019 19:47

remove std::make_pair when not needed

8a81e1d

replace priority queue empty with std::swap

ea44ee0

JDuchniewicz reviewed Dec 7, 2019

View reviewed changes

GGiter added 8 commits December 8, 2019 12:12

fix swap call

d568f18

Merge branch 'dev' of https://github.com/PolyEngineTeam/PolyEngine in…

2c2566b

…to dev

add new wrapper class of priority_queue with clear method

8835713

add missing std::

24a233a

fix compile errors

b5f4920

fix misstypes

72dbbdb

fix clear method compile errors

efa1768

Merge branch 'dev' of https://github.com/PolyEngineTeam/PolyEngine in…

77621c3

…to dev

GGiter requested a review from MuniuDev January 17, 2020 20:19

GGiter added 3 commits January 23, 2020 11:01

Merge branch 'dev' of https://github.com/PolyEngineTeam/PolyEngine in…

8c9b746

…to dev

update priority queue templates

42a6e0e

add priority queue tests

2194cd5

fix priority queue comparator in tests

ffec396

JDuchniewicz reviewed Jan 28, 2020

View reviewed changes

GGiter added 8 commits January 28, 2020 23:35

fix loops

014937d

fix spelling errors

69b7645

fix compile errors

9ceb776

fix comparsion errors

66187a1

change default priority queue predicament from lesser to greater

8c37959

add missing comparator

3c69626

change back default predicament from greater to less

8a5c5ee

change priority queue comparator from less to greater

e0aaea8

MuniuDev approved these changes Mar 29, 2020

View reviewed changes

MuniuDev merged commit 5098ee7 into PolyEngineTeam:dev Mar 29, 2020

		@@ -1,5 +1,6 @@
		#include <pe/core/storage/impl/IndexedStringManager.hpp>

Replace PriorityQueue class with it's stl equivalent #218

Replace PriorityQueue class with it's stl equivalent #218

Uh oh!

Conversation

GGiter commented Nov 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MuniuDev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MuniuDev commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDuchniewicz Dec 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JDuchniewicz Dec 7, 2019 •

edited

Loading