Namespace/File restructuring

[T4Larson]

I not yet sure if I see the recent namespace flooding as a good thing (and with members being added unexectedly in unrelated headers, i.e. script.h), but I'm sure anonymous namespaces in header files serve no real purpose other than to avoid rare and exotic situations, and like here for consts declarations only, this can get only worse: Redeclarations may go unnoticed until they become ambiguous with usage, or even pass completely when someone gets lost in the a "using namespace" and starts using "::SomeConstant" just to make sure... Unless this is some preparation for putting consts in (another) namespace?

[stohrendorf]

That "namespace flooding" is on purpose, and only a first step, because now you can see what is actually unrelated or should be moved between namespaces. My plan is to continue namespace flooding until the files have been roughly organized into modules, and then split these files up so that definitions get into appropriate namespaces. I'm aware this is becoming sort of a mess currently, but it will get sorted out later.

[stohrendorf]

I guess the "namespace flooding" is over. Now comes the tricky part of moving things around in the files and splitting files up into logical units.

[T4Larson]

I'd wish I didn't decide to rebase onto the current master (...and not onto a wip_branchcough), then I'd probably have missed this mess - maybe I've come across too much "messyness" recently that was once meant to "get sorted out later".

However, after thinking about it, I still have issues with this (assuming the structure doesn't change in a couple of commits):

• world:: - this is probably intended to mean the game-world, but imo the "game world" is conceptually the whole structural representation of a level by the game - and wouldn't cover anything that isn't part of the game or the engine. world:: is...too generic, too fuzzy, too global::
• hierarchy is...awkward (world, core, engine): Engine: runs the whole core stuff (input/system interfacing, physics, object and animation management, graphics, resource management, drawing and rendering, ...) Game: _uses_theEngine to show a world where the actual game happens, this is the actual application stuff: object behaviour, game logic/game flow, state processing, player interaction, ... Render: part of engine Gui: part of engine, implementations part of game Audio: engine
  
• The approach seems half-hearted: It currently only bunches together a couple of somehow related source files, and leaves the actual restructuring undone.

Ideally, the sub structure should reorganize the scattered code parts into suitable objects, and then these objects be grouped into namespaces. I'm thinking of something more...concise, like

namespace Game (or Opentomb, or just OT) {
    ... all game related classes
    class Player, Enemy, Pushable,..., Game, Inventory, OTScript, Gameflow, Controls, AI, ...
}

namespace Engine (or just OTE) {
    ... engine classes:
    class Renderer, Gui, OTPhysics, Mesh, Level, Entity, World, Script, Audio, System, Input
}

... and not so very much more. (there's still room to add things later: Engine::Network, Game::ServerBrowser, Editor:: ...)

Especially the Game/Engine separation seems already mostly non-existent in the code, which not only is horrible to maintain or extend, but also has lead to things like a state handling that is peppered with direct object-data manipulation, physics updating and hardcoded engine workarounds every dozen code lines.

Don't get me wrong, I know how this has developed and I'd never dare blaming anyone (except myself should I have remained silent in this issue :wink:). This all may sound harsh (no offence intended!), but the current restructuring imo has much room for improvement, and may deserve more attention to be helpful with getting this mess sorted out.

[stohrendorf]

These are good points, but I'm against reducing the amount of namespaces too much, because I'm thinking about the future when OpenTomb will be able to handle add-ons (HD meshes, non-standard extensions, etc.) that will benefit from a non-monolithic architecture. And the current layout should still be considered a draft, so if you want to move things around now, do it. But please don't put everything together into a single engine namespace :wink:

[T4Larson]

I've tried to go with this refactoring. Eventually, I reverted the namespace mess when I realised that it adds nothing but an extra layer of clutter and disarrangement; Moving sources around in loosely chosen nested directories (and namespaces) to pretend some structure which isn't really reflected in the code - this is cargo-cult programming.

For me, this means right now to keep working without this "structure" is the lesser evil.

And while I'm at it, this just aside: Fragments of wide-scale refactoring of some ongoing WIP straight to master - this isn't exactly good style in a collaborative environment (at least not those I've worked in - the exception proes the rule, hence I know why I disfavour it), same goes for broken and/or untested commits with fix- and refix-noise on master... Please think about it. A lot of suggested "best practices" are more than just some kind of workflow dogma.

I may have failed to wrap all this in nicer wording, but I've been through it before and learnt to value a basic set of collaborative rules and the reduction in time it takes to follow current development, so please indulge me, but we're all not being paid for this.

[stohrendorf]

I see the point in considering this a "pseudo-structure", because it currently is a pseudo-structure. And I agree that usually this refactoring should have been done in a branch, but I considered the conflicts when merging it, so I decided to do it straight on master. I'm completely aware that this was not a "best practice", yet I have seen parallel developments here that would have led to a hardly mergable branch. And this was a pretty safe refactoring, because I only added some namespaces and moved some files around, but did not change the actual code logic.

Resolving that "pseudo-structure" is another series tasks, which I'm going to do on branches, as it is a series of tasks that actually can break the code. Still, I consider the rough layout I put into the code plausible, because you can already see that parts from the loader code bleeds through the whole code right into the render code, which I know is a very bad code smell.

I'm curious, how would you do a code restructuring, or (like in this case) generally begin to give structure to code? Namespaces are a first step in a top-down approach, and everybody is encouraged to move code around to the proper places now. This includes reorganizing that whole "world" folder, which is still a mess (maybe I should have called it "stuff" :wink:).

And about your suggestion to only use "engine" and "game" namespaces: What is the "game", and what is the "engine"? You propose putting the inventory into the "game" namespace, but it is clearly such a general concept of a "container", that it should be moved into the "engine". The same statement holds for the "controls", as they are not directly related to the game itself. In fact, the whole "game" namespace could be moved into the "engine" namespace, as game-specific behaviour is handled by the scripts. The whole software is an "engine", so I decided to split up the engine into its components, having a simple model/view/controller approach in mind.

[vvs-]

This is a matter of experience and personal preference. There exist different abstractions to represent the same structure. The problem is that it can't be imposed on the developers and if someone working on it prefer different abstraction it should be respected by others.

For example many functions could be viewed just as the queries to a game database. Everything could be represented as a database engine, a query protocol and a pipeline to process the results. From that point of view the "engine" is a game independent mechanism and the "game" is a specialization of that abstraction.

[stohrendorf]

Speaking of different points of view, I think it's a better idea to create my own fork of OpenTomb and work on it independently, as I'm obviously doing things that aren't accepted, or even attacked. It's a decision I thought about for a few weeks now and also talked about with other experienced developers, and I am also tired of explaining things that should be obvious. I'm currently working against all of you, not with you. I want to raise the level of software design, but I'm hitting a wall. The problem is that most of you don't want to change things because "they have always been the way they are" and "they work, so don't touch it". I want to touch it all. That's a core problem that's not solvable, so hereby I call @Lwmte or @TeslaRus to revoke my commit rights.

[vvs-]

"they work, so don't touch it".

That's the core principle of engineering :smile:

I want to touch it all.

That's perfectly valid for an experiment but it's a bad idea for a stable branch. And I can recall that it was you who complained about master instability not long ago and asked to stop any experimental changes there until all bugs are fixed. I think that's just as reasonable as working on experimental fork should be.

[Gh0stBlade]

How frustrating, I think you're exaggerating here @stohrendorf not everyone here disagrees with the changes you have made. I fully support the refactoring of the code due to the simple fact it will improve the overall structure and readability I have been craving. I have seen there's a lot of crossfire, opinions, some are happy with one thing, others not. We simply have to discuss this further and come down to a final decision as to what exactly will be restructured, how and a consistent style before work just goes to waste if these changes are reverted.

I've been busy lately, it's just shocking to see that there are persistent complaints about this despite previous discussions. I don't believe that development should be halted until other bugs are fixed. I'm actually 50/0 on this, I would love the master to be stable but I don't think it's going to happen. OpenTomb has some critical bugs which unfortunately haven't been fixed in a long time. Nobody should have pushed any changes if it didn't work properly or was half-baked. If this were the case, it'd never be in this mess in the first place.

Regards.

[vvs-]

We simply have to discuss this further and come down to a final decision

Of course. That's the normal way to do any work. I'd just put the word "final" in quotes, because, well... What could possibly be final in that world?

I don't believe that development should be halted until other bugs are fixed.

I'm fine with that too. But the policy should be consistent. We shouldn't change decisions just because now it's more convenient for anyone to support his current point of view. At least any opinion should be based on facts and not on emotions. There are too much emotions here and too little facts for my taste. That probably explains why there's still no consensus on even basic things.

[stohrendorf]

As I said, I'm out. I'm "socially incompatible", as my boss said with a smile, and I'm not able to express my reasons well enough or to present facts in a way that don't leave things open for misinterpretation. So when do I get my commit rights revoked?

[Lwmte]

It's not very polite of you to make total mess with namespaces and now request to revoke your commit rights. Either roll whole namespace thing back (for ex., to dff0cdf) or finish your work. Because you must understand that no one else will finish this except you.

If you now want so bad to remove you from development, I will do it, but only when you will fix your stuff. I understand that you may have some problems at work, but it's not the reason to ruin everything for everyone else here.

[Gh0stBlade]

There is a lot of fire here, I also sense some passive aggressiveness. Just stop right now please, it's not going to get anyone anywhere. @stohrendorf Has tried to refactor the project, it's just a shame as stated previously most changes are being challenged and thus I foresee a huge revert. Now @stohrendorf is requesting write access removal from the OpenTomb project which isn't looking good so if this is going to be left like this, we're going to have to discuss PROPERLY what we want to do.

@Lwmte Most of the namespace stuff was already confirmed placeholder and work in progress. Why don't you revert the commit rather than requesting @stohrendorf to do so? I agree that if @stohrendorf leaves nobody else will continue it. Reverting to https://github.com/opentomb/OpenTomb/commit/dff0cdf0d134c58753b5dd4e7b68e9e264a926d8 will leave us with a half baked refactored mess which is ridiculous. From what I've gathered @stohrendorf suggested working on his own fork and continuing there, but whether that will be merged into this is unknown. Not sure if he plans to keep all the work there to prevent complaints until it's done or to work there permanently with no means to merge into master.

I've created two reverted branches:

revert-01 - https://github.com/opentomb/OpenTomb/commit/cf50613b62876446c2fde1381fba9d00ba9c5b52 revert-02 -https://github.com/opentomb/OpenTomb/commit/dff0cdf0d134c58753b5dd4e7b68e9e264a926d8

Our options are (if @stohrendorf is leaving):

1. Revert back to revert-01 and recommit everything.
2. Someone should continue the refactoring.

Regards.

[stohrendorf]

The last commit without namespaces from me was f20b6a16f11f65a66e0df9d6082ccbf982e966ec, and at this point -- I'm pretty sure -- there are only bugs which have already been there since before I did my first commit. I am happy to revert these commits myself, because I never thought you would see the current state as a "total mess"; sure, it is a WIP, but it compiles, and it doesn't introduce new bugs. And if I would continue, it would become a real mess, because moving code around in files (instead of putting them only into namespaces) makes it hard for parallel development, because of the merge conflict hell. I can (and will, if I stay) do that in a branch, but that branch had to be constantly merged into master to avoid that; we all remember what happened after my first pull request.

I could try to write a longer document (PDF, HTML, LaTeX, Asciidoc, etc.) where I would put together my current mentality about code flavor (avoid #define, use enum class instead of plain enum, use UpperCamelCase for types and lowerCamelCase for variables/functions, etc etc.), my plans for structuring/refactoring/CXXifying the code, and try to explain why I have these opinions on an objective basis. The thing is, that this would need some hours of work, and the outcome does only have some benefits if and only if the majority wants me to stay. It has to be a clear majority, though, and I'm open for discussions on that document, but I'm not going to abandon my core rules.

Vote: Removed by @Gh0stBlade

[vvs-]

As I said, I'm out. I'm "socially incompatible"

Look, I'm sorry to hear that. I believe you denigrate yourself without reason. You have your shortcomings, no doubt. But who don't? I have no hostility to anyone here. It's just annoying that you are too self-confident sometimes. I just wish that any decision would be grounded on a consensus.

[stohrendorf]

Well, I am socially incompatible, because I am a bit Asperger-authistic... not enough to need help, but enough to be socially incompatible :wink:

[vvs-]

We all socially incompatible in some way. I think that definition doesn't make sense.

[stohrendorf]

Think a second about how it would be if you would have to explicitly learn social behaviour: say "good morning", say "thank you", hug people on their birthday, bring presents, smile, show interest to be polite even if you don't have any, etc.. Most things are so trivial for you, you wouldn't even think that someone had to learn these things, and I guess most people I only meet occasionally think I'm very unfriendly, because I simply forget these things more often than I want to. And most of the time I write or say things "between the lines" that I am not aware of, which leads to problems like this one.

But let's put that aside. There are books about that topic. Let's see where the poll is going, but I believe this will be one of my last posts in this repo.

[vvs-]

I can (and will, if I stay) do that in a branch, but that branch had to be constantly merged into master to avoid that; we all remember what happened after my first pull request.

And that's the original reason for that issue. People want to base their work on a stable branch, otherwise it cause them many frustrating hours just to return back to a working state. We need to stop arguing about emotional stress and find a solution to the real technical problem. There are definitely a conflicting goals here: we want it to be stable to easily merge the changes back to master, but we want to refactor things too.

[Gh0stBlade]

I don't see any difference with before the merge/after with the refactoring. All the bugs have already been fixed. OpenTomb has never been stable, it's always consisted of features that were implemented and not fully completed. That's ok to get things up and running but it's time to lock this stuff down and fix the problems as well as adding new things.

[vvs-]

Well, that means that you didn't touch much of the changed code. But think about T4Larson who is trying to refactor code as well. He has huge conflicts on his side. We need to set some ordering policy for any refactoring work, so they wouldn't step on every other toes.

[vvs-]

most people I only meet occasionally think I'm very unfriendly, because I simply forget these things more often than I want to

You'll be surprised to know how little I care about social behavior. I always tried to suppress any opposition at work and many times I succeeded. But most people won't be angry at you if you just try to listen to them and respect their opinion even if you don't agree.

[stohrendorf]

@Gh0stblade decided. I'm going to revert the namespace stuff, most of you seem to be happy without it.

[vvs-]

most of you seem to be happy without it.

I think you take it too personally. It's premature, people just not ready yet. Just wait until it becomes real problem for them.

[Gh0stBlade]

Ok so, NO MORE POLLS ABOUT WHO SHOULD STAY OR NOT. Unless it's to do with actual OpenTomb features, go for it. I'm not having silly polls posted here and refuse to participate in them @stohrendorf Whether we think you should leave or not is irrelevant, nobody should be suggesting anyone leaves. If you truly and still care for the project you'd stay otherwise you'd leave.

This is NOT our decision to make, don't try make us chose like we're the ones driving you away. I like the commitment you have made and it would be a shame if you left. But your actions are starting to become unacceptable and disappointing. Stop trying to isolate yourself from us, we're a team. I understand that you feel everyone is against you, there's lots of annoyance due to the refactoring (which I cannot stress enough everyone should understand that it's W.I.P!). This is not the case, I fully support the refactoring of OpenTomb to this day. We all have our own opinions on how it should be done, each one of us could refactor it differently and someone else may not like that. It's just how it is unfortunately.

To be honest, I haven't looked at the namespaces that were added. Do I like namespaces? Yes, if they're appropriately used (not suggesting that they're not in this case). This could have been easier if we'd all have agreed on what exactly needed to be done rather than leaving it to @stohrendorf then complaining. @stohrendorf If you can get back to the group with the style you intend to utilise in more detail this will be more easier to discuss even though that may take some time.

I'm finding this extremely immature, we're adults here so we should act like them.

[vvs-]

Thanks, Gh0stBlade. I really like what you wrote and agree wholeheartedly!