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

Connect to 20Minutes, web socket on same port #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tcoupin
Copy link

@tcoupin tcoupin commented Mar 19, 2020

No description provided.

Copy link
Owner

@benVigie benVigie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, merci pour l'ajout ! J'ai créé ce jeu il y a 6 ans pour y jouer avec des amis, et ça fait vraiment plaisir de voir que des gens l'aiment et y jouent encore 😄 !

J'ai noté quelques petites choses dans ta MR, mon plus gros concern étant surtout qu'il reste utilisable pour le plus grand nombre en local et avec un maximum de fonctionalitées 😕. Si tu as le temps de régler ça, alors ta MR est la plus que bienvenue. Encore merci a toi 😉!

Comment on lines -4 to -5
"SOCKET_ADDR": "http://127.0.0.1",
"SOCKET_PORT": 1337,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si tu commentes ça, les autres joueurs ne pourront plus se connecter non ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ils pourront car la socket.io utilise le même port que le web "standard" cf https://github.com/tcoupin/mots/blob/master/game_files/motsFleches.js#L201

@@ -104,8 +104,50 @@ function parseGrid(callback, serverText) {
nbWords: 0,
cases: []
};

eval(serverText); // Parse and evaluate js, generate variable gamedata
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ce qui me dérange un peu avec cette approche, c'est qu'on enlève complètement la possibilité d'avoir les grilles du Metro pour le remplacer par celles du 20mn.

Plutôt que de perdre tout un provider, penses-tu que tu pourrais extraire les 2 logique dans 2 fichiers séparés ? Ainsi le gridManager irait lire quel est le provider dans le fichier de config et chargerait le bon loader de grille selon qu'on veuille jouer Metro ou 20mn !

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'avais compris que Metro ne fonctionnait plus...
On pourrait effectivement garder les 2 formats, je viens d'ailleurs de voir que NotreTemps utilisait aussi ce format : https://rcijeux.fr/drupal_game/notretemps/mfleches/grids/mfleches_1_1730.mfj

Potentiellement beaucoup de grille :)

@@ -179,7 +221,7 @@ function parseGrid(callback, serverText) {
}
}
};

*/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Donc toute cette "logique Metro" se trouverait dans un autre fichier aussi...

@@ -12,27 +12,6 @@ define(function () {
}


function injectInGameInfoPanel() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Donc y a pas d'infos du jeu avec 20mn (j'ai pas encore pu tester 😃) ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Le soucis c'est que 20minutes a 2 panneaux dans ses grilles et que la div créée recouvre l'ensemble donc cache des cases. J'ai passé du temps à essayer de détecter des groupes de cases pour faire 2 div mais je n'ai pas réussi. Du coups j'ai préféré mettre les cases en gris et déplacé le panneau d'info sous les scores

/** Call when the express server has started */
async function onServerReady() {
console.log('Express server listening on port ' + app.get('port'));

var addresses = getLocalIpAddresses();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pourquoi enlever cette feature ? Est ce que t'héberges le jeu quelque part ? Si oui je veux l'adresse 😄 !!

Mais sinon d'un point de vue fonctionnalités, faut laisser aux gens la possibilité de le lancer en local. Je te proposerai plutôt encore une fois de laisser une option dans le fichier de configuration qui force une adresse et bypass ce choix par défaut.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parce que je travaille avec Docker qui isole le processus et la partie réseau. De toute façon cette fonction n'était utile que pour la websocket qui est fusionée avec l'app expressjs. Et l'app expressjs écoutait déjà sur toutes les ips

@tcoupin
Copy link
Author

tcoupin commented Mar 20, 2020

Merci pour cette belle appli, j'avais besoin de ça pour continuer à se divertir avec les collègues pendant notre période de télétravail forcé (covid19...).
D'ailleurs c'est voulu que des nouveaux joueur ne peuvent pas rejoindre la partie quand elle est commencée ?
Je t'envoies l'adresse de mon serveur (raspberrypi) en mp sur twitter

@hverlin
Copy link

hverlin commented Mar 21, 2020

@tcoupin on a un fork ici https://github.com/Drestin/mots qui permet de rejoindre une partie en cours si ça t'intéresse ! (il marche aussi sur téléphone maintenant).

On voulait aussi intégrer d'autres sources maintenant donc ça tombe bien.

@benVigie
Copy link
Owner

Salut @hverlin !

C'est dommage d'avoir forke le projet et develope de nouvelles features dans ton coin ! Quand j'ai developpe ce jeu, je l'ai mis en ligne sur Github afin que tout le monde puisse s'amuser avec et participer a son developpement. Du coup je trouve dommage que tu developpes des features interessantes sur ton fork plutot que sur le projet original car tu les gardes pour toi au lieu de les paratager a tout les gens qui ont deja le jeu.

De plus quand quelqu'un rajoute une fonctionalite comme c'est le cas avec @tcoupin, tu dois rebase ton fork et gerer les conflits plutot que de simplement pull la master branch 😕.

@hverlin
Copy link

hverlin commented Mar 23, 2020

@benVigie Oui oui, nous voudrions intégrer les nouvelles fonctionalités ici.

Nous avons encore pas mal de choses à corriger avant de pouvoir faire une bonne PR (en particulier le score ne marche pas encore correctement avec + de 5 utilisateurs donc on l'a temporairement enlevé).

On voulait juste signaler qu'on avait commencé à corriger quelques uns des problèmes mentionnés ici :)

@benVigie
Copy link
Owner

Super cool @hverlin 😉 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants