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

Render engine, closes #26 #66

Merged
merged 76 commits into from
Feb 1, 2018
Merged

Render engine, closes #26 #66

merged 76 commits into from
Feb 1, 2018

Conversation

Abergard
Copy link
Member

@Abergard Abergard commented Oct 26, 2017

This change is Reviewable

@Abergard
Copy link
Member Author

Review status: 72 of 84 files reviewed at latest revision, 18 unresolved discussions.


cmake/platforms/Linux.cmake, line 1 at r3 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

"Detected GNU CXX compiler.")
Shouldn't this be in cmake for GCC?

Done.


src/graphics/core/RendererPoolSfml.ut.cpp, line 148 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Could you write what should happen in this scenario?

Done.


src/graphics/core/SfmlRectangleShape.hpp, line 4 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Do you use it?

Done.


src/graphics/core/SfmlRectangleShape.ut.cpp, line 24 at r1 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

What about less then?

I removed them. We don't need to compare shapes in that way now. Done.


src/graphics/core/SfmlRenderTarget.ut.cpp, line 21 at r1 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Please rename test to more descriptive.

Done.


src/graphics/core/WindowSfml.ut.cpp, line 77 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

prepare_events returns int, maybe rename to number_of_expected_events?

Done.


src/graphics/core/WindowSfml.ut.cpp, line 91 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

prepare_events returns int, maybe rename to number_of_expected_events?

Done.


src/graphics/includes/graphics/Window.hpp, line 8 at r3 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…
Window() = default; // Q: Is it a good idea
                    // to add default constructors to Interface
Window(const Window&) = default;
Window(Window&&) = default;

I don't thing we should add any constructor at all, this interface does not have any member, so see no need for adding it.

Done.


src/graphics/types/Position2f.hpp, line 15 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Maybe delegate to own constructor to avoid code duplication?

Done.


src/graphics/types/Position2f.hpp, line 19 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Maybe delegate to own constructor to avoid code duplication?

Done.


src/graphics/types/Size2f.hpp, line 16 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Maybe delegate to own constructor to avoid code duplication?

Done.


src/graphics/types/Size2f.hpp, line 20 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Maybe delegate to own constructor to avoid code duplication?

Done.


src/graphics/types/WindowSize.hpp, line 16 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Maybe delegate to own constructor to avoid code duplication?

Done.


src/graphics/types/WindowSize.hpp, line 20 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Maybe delegate to own constructor to avoid code duplication?

Done.


src/math/includes/math/Position2f.hpp, line 29 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Please consider extract std::tie(*) to avoid code duplication

Done.


src/math/includes/math/Size2.hpp, line 24 at r4 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Please consider extract tie(*) to avoid code duplication

Done.


src/graphics/src/SfmlRectangleShape.cpp, line 10 at r1 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

OK

Not applicable any more.


Comments from Reviewable

@kn65op
Copy link
Collaborator

kn65op commented Dec 21, 2017

Reviewed 5 of 90 files at r2, 9 of 61 files at r4, 21 of 21 files at r6.
Review status: all files reviewed at latest revision, 18 unresolved discussions.


Comments from Reviewable

@kn65op
Copy link
Collaborator

kn65op commented Dec 21, 2017

:lgtm:


Review status: all files reviewed at latest revision, 18 unresolved discussions.


Comments from Reviewable

@kn65op
Copy link
Collaborator

kn65op commented Jan 25, 2018

Reviewed 2 of 90 files at r2, 2 of 61 files at r4, 15 of 17 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/game/SimpleMap.cpp, line 14 at r7 (raw file):

    {
        physics_ids.push_back(physics_engine.register_colider(
            {static_cast<float>(generated_walls.second.first),

I assume that would be better that generator generate floats.


src/graphics/core/SfmlRenderTarget.ut.cpp, line 12 at r7 (raw file):

namespace graphics
{
class RenderTargetBaseFunctionsTest : public Test

Interesting solution, however I am not sure if we want to test wrappers.
Let's wait for others to discuss.

I think we can keep it, but think if other test like this are required.


Comments from Reviewable

@Abergard
Copy link
Member Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/game/SimpleMap.cpp, line 14 at r7 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

I assume that would be better that generator generate floats.

It needs more time to change current implementation. For now I will leave it as it is.


src/graphics/core/SfmlRenderTarget.ut.cpp, line 12 at r7 (raw file):

Previously, kn65op (Tomasz Drzewiecki) wrote…

Interesting solution, however I am not sure if we want to test wrappers.
Let's wait for others to discuss.

I think we can keep it, but think if other test like this are required.

Ok.


Comments from Reviewable

@Abergard Abergard added this to In Progress in Bomb #01 Jan 27, 2018
@Abergard Abergard added this to the 0.1 milestone Jan 27, 2018
@Abergard Abergard added the Task label Jan 27, 2018
@Abergard Abergard changed the title Render engine Render engine, closes #26 Jan 27, 2018
@Abergard Abergard merged commit c8776ba into master Feb 1, 2018
@Abergard Abergard deleted the RenderEngine branch February 1, 2018 07:10
@Abergard Abergard moved this from In Progress to Done in Bomb #01 Feb 1, 2018
kn65op pushed a commit that referenced this pull request Feb 1, 2018
* add renderer sfml - prework

* add renderer sfml - prework 2

* add renderer sfml - prework 3

* rework sfml renderer and add gmock

* something is wrong...

* something is wrong... v2

* save it

* it works...partially

* add examples

* try fix window compilation

* add sfml lib dir for appveyor

* try fix linux compilation

* try fix linux compilation - add this

* add factory

* reorganize files

* Reorganize directories and replace math structures by own

* Reorganize directories part two

* cmake make global standard requirements

* fix std14 standard for linux

* ut

* update glm from 0.9.5.3 to 0.9.9-a1

* update GSL to master

* update range-v3 from 0.2.1 to 0.3.0

* update docs

* ut

* fix dependency

* fix dependency

* external up

* refactoring example

* refactoring

* fix warnings

* merge master fix

* not working yet, but commit need to be done

* still in progress

* add Window to graphics

* remove disabling warning

* update imgs

* fix sfml enum in test

* add boost operator

* update docs

* update ClassDiagram md

* appveyor show lib lis

* review fix

* fix appveyor

* merge with master part two

* fix window crash

* fix warnings

* fix test coverage linker

* make coverage global

* delete src directory and update docs

* add window test suites

* sneaking around

* sneaking around

* sneaking around

* sneaking around

* sneaking around

* sneaking around

* sneaking around

* sneaking around

* fix not overriden virtual function

* boost boost

* fix unused-template

* fix game ut compilation

* fix and restore game ut

* introduce math lib to physics interface

* fix physics cmake

* try to disable coverage for facade function

* try to disable coverage for facade function

* change SfmlRenderTarget to template class and create stub for testing purpose
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Bomb #01
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants