V0.1.0 - First working demo with cleaned up code #2

gbrivady · 2022-12-04T03:13:42Z

EDIT : no need to check memory leaks, will be adressed separaretly

The PR to start anew on a cleaner basis, finally!

Main goal of after the PR is to scale the project, add new nembers to the team, and start working on funnier stuff ✨

Lot of work still left to do, and some pretty ugly code/tape still running in some places; but should be mostly okay to build on.
Two aspects need to be adressed: a technical one, to make sure the code is ready to be merged, and a more subjective one, as we should use this milestone to check that the conception is going in the right direction.

If you only want to make sure everything is working, simply go throught the Technical section. If you want to know more about how everything works, read everything. Everyone read the last words tho!

Project structure

Headers file are in the include folder, source code in src. CMake files are recursive to each folder to prepare for the complexity of the project structure. Executables are built to the bin folder - not included in the git repository.

Game, Engine, Graphics, Others

I have tried to separate source files in three big folder. The goal is to split as much as possible each "main system", so we have the option in the long term to create independant projects and reuse separate components on other projects.

Engine

Should contain most of the generic, engine-type stuff. A lot of virtual classes, the physic engine, and most of the internal logic of the project.

Game

Everything that will be game-specific, ie game characters, Entity class implementations, components implementations, ...
The dev folder give the "spirit" of what should go there.

Graphics

Not to be a focus for quite some time! Everything fancy that will be required down the road, including f.e. texture loading, more advanced displaying, maybe particles, ...

Others

Currently only keybind loading. But we should probably put there saving, preferences, options loading as a whole, etc...

Technical

The following technical things need to be checked before we proceed with the project:

Readme is clear enough
build instructions are correct and clear enough
building works on other platforms (ie Linux), other compilers... especially important since ideally, a lot of people are going to work on the project
code is clear and clean enough;
and documented enough to be understandable
POTENTIAL MEMORY LEAKS
not required, but look at access control if possible

I believe items in bold are the minimal requirement to accept the PR, the rest is "somewhat" secondary.

Some precision on code clarity and "cleanliness": everything that has a TODO attached is not meant to be adressed with this PR. We can still discuss it ofc, but they will be adressed afterwards as issues, for the following reasons:

Some stuff require a lot more conception and work, and will stay as-is for now;
Some other stuff could be excellent entry-point for new members into the project, especially for the first years with minimal OOP knowledge.

Conception

On a more subjective point, we should take some time to discuss whether or not the project is flexible enough to be build on.
This will require a lot of discussion (that we will keep out of this PR, unless you see an obvious flaw that can be fixed), but it would be nice if everyone could think a little bit about this while reviewing the code.
To help with this, I have attached an elided UML of the current state of the project at the end of the PR.
I try here to brief on all the subsystems.

Graphics/Display

Prettry straighforward. Straight pipe between the engine and the SFML library

Box

System that will handle intersection of shapes. Currently used for collisions, soon to be used for the combat system (hitboxes and hurtboxes f.e.). Only implements rectangles as making several shapes interacts is a big headache, and is not exactly required as of today.

Physics

Contains a simple gravity to make objects fall; and a not so simple subsyste,

Input

Simply translates keys to entity movements. Very very basic, lacks a lot of stuff including a jump subsystem, proper buffering of input, animations, etc...

Universe Master

I think this is the strongest point of the conception. I believe the code illustrates by itself the beauty and simplicity of addind/removing/updating component to the game.

Entity

Keystone of our Entity-Component-System, yet I fear it is already outdated. Please pay attention to this class in particular and think about way to further separate the entities and the components that I find currently too linked.

UML diagram

Not complete, lacks a LOT of methods, attributes, access control...

Last words

Quite some work done, but more to come!
Thanks everyone for thrusting me with the project cleanup, and sorry it took so much time to get there

But I believe the "fil rouge" will finally start for real, 1 year after we quicklaunched it!

Very lenghty PR, but I think it is coherent with the amount of modifications. If I forgot something, or you want to do some remark on the PR (content, form, ...) don't hesitate to do so.

…lable

…bUpdate Hitbox and Collision: libs, typos, convention OK | checkCollisionEntities TODO updated to new hitboxes collision feature to be finished by gregory, missing singleCheckCollision libs OK, parameters and return type OK, missing PhysicalEntity.getForces implementation hitbox / readme more reasonable hitbox / readme mise en page hitbox / readme dernière mise en page en théorie

…tt-merge

amtoine · 2022-12-12T18:06:11Z

@gbrivady
thanks a lot for the thread resolutions, that was really pleasing and easy 😌 🤩

only one minor thing in the remaining thread
and the major things that were not addressed in the review will be in Formatter to match our coding convention #14

so once the thread above is resolved, i'll be more than happy to approve your changes and this PR 😤

amtoine

with all the threads i've opened now resolved and as i checked all the commits, this PR is ready for me 💪 🚀

ctmbl · 2022-12-19T18:06:20Z

@gbrivady
I've already said it in private, but will say it once again:

a HUGE thanks ❤️ for your work

I think I'll not mistaken saying that without you this project would have never rose again! And that you accomplished exactly what it needed: a clean modular basis on which to build together.
that being said let's review 💪

ctmbl · 2022-12-19T18:20:01Z

@gbrivady @amtoine

⚠️ dead code

Why is it still a ⚠️ is there still some dead code?

Left Physic as-is, if you think we should we rename it to Physical say so

not sure I agree with this 🤔 according to WordReference 'Physic' means 'medicine/drugs' and 'Physical' means physique (charnel/corporel) (in French) then I think 'Physics' is the appropriate term and we should use it

Also, I think amtoine didn't ask to remove the many empty lines in the README.md, this is detail level and can be address in #13 but there are quite a lot!

As for the other threads and points @amtoine arose I agree with them all so tks to both of you!

Personally I found the README quite explicit but as discussed in #13 we'll be able to improve it sooner or later 👍
The project built and run perfectly for me as soon as I installed SFML.
Didn't check memory leaks however 😕

ctmbl

Some questions, but of course I didn't review all of it 😕 and I clearly lack knowledge of Cpp and cmake.
However I can't wait for your presentation of the project!

PS: should you really use classic pointers in cpp?

include/engine/box/box.h

src/CMakeLists.txt

src/engine/box/CMakeLists.txt

src/engine/physics/physic.cpp

amtoine · 2022-12-20T08:12:23Z

warning dead code

Why is it still a warning is there still some dead code?

no no that's been addressed, i just left the comment as-is 😉

not sure I agree with this thinking according to WordReference 'Physic' means 'medicine/drugs' and 'Physical' means physique (charnel/corporel) (in French) then I think 'Physics' is the appropriate term and we should use it

👍

Also, I think amtoine didn't ask to remove the many empty lines in the README.md, this is detail level and can be address in #13 but there are quite a lot!

i do not see empty lines in the README 🤔
i'm currently on the tip of the branch, edb5533

gbrivady · 2022-12-20T15:04:16Z

not sure I agree with this 🤔 according to WordReference 'Physic' means 'medicine/drugs' and 'Physical' means physique (charnel/corporel) (in French) then I think 'Physics' is the appropriate term and we should use it

Is Physic wrong? Yes. Keep in mind that we are talking about the component of the entity that make it obey the laws of Physics (as submodule of the engine which has already been renamed accordingly).
As for Physical, according to the Cambridge Dictionnary : "relating to things you can see or touch, or relating to the laws of nature"
"relating to physics:"

so when we are talking about the component, Physical is I believe the right term. Now, I did not changed it because it is a pretty tedious change, but if you agree with this, and this you are both bothered by it (- and me too tbh) I will probably rename it accordingly too.

amtoine · 2022-12-20T18:35:06Z

@gbrivady @ctmbl
let me try to wrap this 😋

we are trying to find a good name for

the directory src/engine/physics/
the file src/engine/physics/physic.cpp
right?

what about a simple src/engine/physics/ and src/engine/physics/physics.cpp or src/engine/physics/main.cpp? 😋

gbrivady · 2022-12-20T18:50:10Z

The directory is correct, as it refers to the subsystem (ie the physics)
But I don't like the idea of using the system name for an entity component

amtoine · 2022-12-20T19:03:32Z

The directory is correct, as it refers to the subsystem (ie the physics) But I don't like the idea of using the system name for an entity component

ok i see

imo, the problem with physical, if any, is that it is not a noun and thus is not coherent with the rest of the code base

look at all the .cpp source files, it's the only one like this 😮

ideas

main: the drawback of this is that it "conflicts" with the "real" main.cpp file of the game 🤔
physics: this "conflicts" with the root directory of the file itself
regarding what the files appears to be doing
- apply.cpp
- forces.cpp
- application.cpp
- law.cpp
- balance.cpp
- update.cpp

that's what comes to my mind as for now 😌

gbrivady · 2022-12-20T20:36:05Z

How about particle.cpp? Not exactly correct, but we are doing point cinematics and changing to solids or more complicated stuff would probably require another class in itself, so we could go with this. I think it is enough of an hint to how the component is supposed to behave, and a name so coherent with the rest

amtoine · 2022-12-20T21:28:29Z

mm
particle.cpp or point.cpp then 😋

gbrivady · 2022-12-22T01:40:32Z

I think I will go with particle.cppthen, if @ctmbl is also fine with it (mentionning since you suggested the change)

PS: should you really use classic pointers in cpp?

Also mentionning to answer this : absolutly not, will do a full patch (0.1.1) after the PR to replace everything with smart pointers to fix all the leaks

ctmbl · 2023-01-06T22:30:21Z

@amtoine @gbrivady
Ok I really like your arguments here tks for taking seriously my point!
I'd prefer point.cpp I guess but I'm fine with particule.cpp as well so please go with the one you want!

gbrivady · 2023-01-08T17:19:17Z

@ctmbl Naming fixed in f72436c, should be go to merge now

amtoine

two things:

the renaming of the files is fine 👌
i do not like the change about

// in include/engine/box/box.h
typedef std::pair<double, double> point;

and

// in include/engine/physics/point.h
class Point

it looks to me like there should be only a single "point" type, but still keep the typedef to make include/engine/box/box.h easier to read 🤔

gbrivady · 2023-01-09T19:16:36Z

it looks to me like there should be only a single "point" type, but still keep the typedef to make include/engine/box/box.h easier to read thinking

To be clear: I also believe it should be the case, but I will go more in depth on why I removed it (tldr at the end):

Three identifiers are named some variation of point in the code:

std::pair<double, double>
The Point class from the physics module
Its member within the entity class, point of type... Point, and by extension all the "simple" Point objects

(1 or 3) and 2 are not ambiguous : the case is not the same, but 1 are and 3 are, and since entity.h includes box.h at some point, the conflict arises.

I saw three way to fix it:

Rename the point that is a member of Entity, and make more explicit name for any occurrence where point is the name of a variable
Remove the type point from box.h
Use namespaces

I chose not to go with 1, to keep coherence within Entity: for each component class we add, name the member as its type - which is a thing than can be discussed on its own;

3 is obviously the way to go, and definitely the long term target: use namespaces for every module, to make sure there is not naming ambiguity. Obviously did not go with that, because it is a pretty tedious change, and I believe we should do it incrementally until everything is namespaced (issue planned to keep track of what is namespaced/not namespaced);

Since 2 is the easiest and quickest fix, and the box-subsystem is not too complex yet, I preferred to go with that.

To be clear, there is probably another solution (for example don't use a constructor that relies on the point type in Entity), but once again, the goal is to merge this first, then fix, so yeah.

TLDR: while we wait for everything to use namespaces, it was the easiest way to fix a naming conflict.

Edit : I can always re-add with a proto-namespace (ie something like box_point) if you really think we should keep it while waiting for namespaces

amtoine

@gbrivady, thanks for the explaination 😋

that's fine with me if it's planned in the future 😌

EDIT: and the source still compiles with the two last commits, so that's perfect 👌

ctmbl

Everything's fine to me, let's merge it!

…ts (#16) This PR should close #6. Requires - #2 to be merged --- This PR adds a slightly modified default action from _GitHub_ for `CMake`-based projects 👍 I did run the pipeline on my personal fork, which resulted in [this action log](https://github.com/amtoine/wiresmash/actions/runs/3670185074/jobs/6204543791) on the tip of the branch of #2 👍 this means that the tests will pass when the PR is ready 💪 ## NOTES - this pipeline won't work on `windows` hence the single `ubuntu-latest` in the matrix of OSes 🤔 - i've let the comments from the default configuration file, just for reference 👍 - there are not tests to run, so they pass 👌

ctmbl and others added 30 commits January 26, 2022 18:19

add gitignore

b1465d9

modify gitignore

818f8d4

multiDrawable class should work

de6ec17

feature implemented v1

59d836e

Initial commit : strategies constructors with and without default values

bbe0efc

Character default strategies, + some fixes in multi_drawable & schedu…

8e9dff4

…lable

Finished constructors

15978b2

Wall constructor

394cbbe

Résumé constructeurs entities

0327d35

First commit

a19e0b4

Adjustments for entity displacement

fe07388

Should work

4d5f64e

Added readme and polished code

bea88fe

some addition of files, getters and setters in the entity part.

29da4db

add and remove functions for vector in entity part.

ec7e2d5

feature implemented v1

54c1334

feature implemented v1.1

c15eeee

Not finished correction ETT

7f002e7

commit de readme de ses morts

36cdc3c

multi-draweable moved

18025ed

Merge branch 'ett-merge' of https://github.com/iScsc/wiresmash into e…

4d9ae43

…tt-merge

Starting working on main

775f385

damageable / .h et .cpp corriges

5b4bc7e

controllabe should work: to be modified to use linked list

d861302

Correction lib/lib vel and pos. Correction include issues

60b25cf

Correction numerous includes and littles bugs. Sketch pre_main

71b75b3

quelques modifs sur multi_damageable.h/.cpp

417ccca

quelques modif sur physical_entity.h/.cpp

85a918f

très légère modif su strategy.h/.cpp

a54b428

Fix cinematic to kinematics in the documentation

e32a5e2

amtoine mentioned this pull request Dec 12, 2022

Formatter to match our coding convention #14

Closed

4 tasks

Update README.md

edb5533

amtoine approved these changes Dec 12, 2022

View reviewed changes

ctmbl reviewed Dec 19, 2022

View reviewed changes

include/engine/box/box.h Outdated Show resolved Hide resolved

src/CMakeLists.txt Outdated Show resolved Hide resolved

src/engine/box/CMakeLists.txt Show resolved Hide resolved

src/engine/physics/physic.cpp Outdated Show resolved Hide resolved

gbrivady added 2 commits December 20, 2022 15:51

Remove dead include from box

edd0351

Remove useless SFML package from cmake

a0a25cd

gbrivady added 2 commits January 8, 2023 18:12

Remove point typedef to prevent name ambiguity

2a9de19

Rename "Physic" to "Point"

f72436c

amtoine self-requested a review January 9, 2023 18:20

amtoine requested changes Jan 9, 2023

View reviewed changes

amtoine approved these changes Jan 10, 2023

View reviewed changes

ctmbl approved these changes Jan 16, 2023

View reviewed changes

amtoine merged commit 10f8a0c into main Jan 18, 2023

amtoine deleted the a_new_beginning_✨ branch January 18, 2023 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V0.1.0 - First working demo with cleaned up code #2

V0.1.0 - First working demo with cleaned up code #2

gbrivady commented Dec 4, 2022 •

edited

Loading

amtoine commented Dec 12, 2022

amtoine left a comment

ctmbl commented Dec 19, 2022

ctmbl commented Dec 19, 2022 •

edited

Loading

ctmbl left a comment

amtoine commented Dec 20, 2022

gbrivady commented Dec 20, 2022

amtoine commented Dec 20, 2022

gbrivady commented Dec 20, 2022

amtoine commented Dec 20, 2022

gbrivady commented Dec 20, 2022

amtoine commented Dec 20, 2022

gbrivady commented Dec 22, 2022 •

edited

Loading

ctmbl commented Jan 6, 2023

gbrivady commented Jan 8, 2023

amtoine left a comment •

edited

Loading

gbrivady commented Jan 9, 2023 •

edited

Loading

amtoine left a comment •

edited

Loading

ctmbl left a comment

V0.1.0 - First working demo with cleaned up code #2

V0.1.0 - First working demo with cleaned up code #2

Conversation

gbrivady commented Dec 4, 2022 • edited Loading

EDIT : no need to check memory leaks, will be adressed separaretly

Project structure

Game, Engine, Graphics, Others

Engine

Game

Graphics

Others

Technical

Conception

Graphics/Display

Box

Physics

Input

Universe Master

Entity

UML diagram

Last words

amtoine commented Dec 12, 2022

amtoine left a comment

Choose a reason for hiding this comment

ctmbl commented Dec 19, 2022

a HUGE thanks ❤️ for your work

ctmbl commented Dec 19, 2022 • edited Loading

ctmbl left a comment

Choose a reason for hiding this comment

amtoine commented Dec 20, 2022

gbrivady commented Dec 20, 2022

amtoine commented Dec 20, 2022

gbrivady commented Dec 20, 2022

amtoine commented Dec 20, 2022

ideas

gbrivady commented Dec 20, 2022

amtoine commented Dec 20, 2022

gbrivady commented Dec 22, 2022 • edited Loading

ctmbl commented Jan 6, 2023

gbrivady commented Jan 8, 2023

amtoine left a comment • edited Loading

Choose a reason for hiding this comment

gbrivady commented Jan 9, 2023 • edited Loading

amtoine left a comment • edited Loading

Choose a reason for hiding this comment

ctmbl left a comment

Choose a reason for hiding this comment

gbrivady commented Dec 4, 2022 •

edited

Loading

ctmbl commented Dec 19, 2022 •

edited

Loading

gbrivady commented Dec 22, 2022 •

edited

Loading

amtoine left a comment •

edited

Loading

gbrivady commented Jan 9, 2023 •

edited

Loading

amtoine left a comment •

edited

Loading