Override method "IsPlayerShip" for player

[mike-f1]

Adding IsPlayerShip in class player

[laarmen]

May I ask what is the purpose of this? The overriden method isn't used anywhere AFAICT.

Also, as stated elsewhere, please add more details :-)

[mike-f1]

I think is a step ahead for the issue #38 :
because if you don't inherit player from ship then you cannot use their type.

[mike-f1]

But if not, I think you have to remove IsPlayerShip from DynBody...

[laarmen]

Yeah, that'd be my approach. You'd have to change the interface anyway when the ship /player divorce happens. Le ven. 6 janv. 2017 11:03, mike-f1 <notifications@github.com> a écrit :

…

But if not, I think you have to remove IsPlayerShip from DynBody... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3900 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkONXC7AcpE-ryLfTIZQvCdgJOc4OdOks5rPhF4gaJpZM4LcCv7> .

[ecraven]

I don't think this is a good idea, with these changes we could have a Player for which IsPlayerShip returns false... We should go through all places that check whether a ship is the player and fix them to use IsPlayerShip before merging this.

[laarmen]

@ecraven, which one exactly is the bad idea? The original one from the PR, or the IsPlayerShip removal?

For now I'm fairly against merging the PR, but that's mainly gut feeling.

[ecraven]

I think this should be part of a larger effort to separate Player from Ship in the inheritance hierarchy. But that is a fairly large undertaking in itself :-/ The changes proposed in this PR to me seem to muddy things further, we would have two ways of checking for the player, inheritance and IsPlayerShip, which can easily disagree :-/

[laarmen]

Thank you, that's exactly what caused my uneasiness (is that a word ?) with the PR.

I commend the effort, though.

[ecraven]

Absolutely, the idea is very good, but I think we need to think about the general architecture, so we don't run into even more problems than we have already :-/

[fluffyfreak]

To me this makes more sense than our existing stuff. Either the IsPlayerShip needed removing, or it needs fixing. This fixes it.

[ecraven]

I'd advocate removing it (as it seems to be entirely unused) and re-introducing it when we get around to breaking the inheritance chain of Player to Ship.

[mike-f1]

I think the problem is (...or I hope is ) not on these few lines (it works either ways...): the problem is if someone would start changing checks btw Player and Ship spread on the code here and there (also for a little commit), then he could start; else you put the whole thing (the "split" and the "checks" ) in only one commit.

[laarmen]

I'm not sure I understand this right, but if I do, then when you split Ship and Player you intend to put the knowledge of being player-owned in the Ship object? If so, I think that is the wrong location for this information, the Player should be the one holding the knowledge. About the content of the PR itself, I'd *really* like it better if it removed the method.

…

On Tue, Jan 10, 2017 at 4:42 PM mike-f1 ***@***.***> wrote: I think the problem is (...or I hope is ) not on these few lines (it works either ways...): the problem is if someone would start changing checks btw Player and Ship spread on the code here and there (also for a little commit), then he could start; else you put the whole thing (the "split" and the "checks" ) in only one commit. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3900 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkONZJXnmvpF56ofi6dqJ73bV3rgoAXks5rQ6bdgaJpZM4LcCv7> .

[mike-f1]

@laarmen : IMHO, ships should know if they are owned by player or NPC...

But even if not, 2 things should be be taken into account if you make player a totally different kind of class:

1. the type based check will not work.
2. all actual stuffs that rely on the fact that Player IS a Ship needs changes

About the second point I think something like:
IsPlayerShip() { return ( is_this_ship_own_by_the_player ); };
even as a temporary solution, could be enough to allow actual stuffs works AFTER the split because the check they do is already working (I mean if based on the method).
Then, you could think on how to avoid putting this knowledge on Ship...

That said, for the effort I put doing this change, I think I can remove both methods.

Let me know

[ecraven]

IMHO, ships should know if they are owned by player or NPC...

I am not an expert at game design, but I'd tend to disagree here. A ship should not do other things than be a ship. The Player class (or even better component) should do all that. Every time you need to know whether a Ship is a player inside Ship methods, you should have a component or function outside of Ship to call and handle that. At least that's what I read.

[robn]

That's exactly what PlayerController (I think it was called that) is supposed to do. Ship, DynamicBody, etc should represent the physics object. *Controller is what links the physics into other systems, the UI for the player, the AI for NPCs, and so on.

(It was never that clean when I last worked on it, but that was the broad direction we were moving in).

[fluffyfreak]

Closing since we chose to remove rather than augment this.