Skip to content

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

Open
tcoupin wants to merge 2 commits intobenVigie:masterfrom
tcoupin:master
Open

Connect to 20Minutes, web socket on same port#34
tcoupin wants to merge 2 commits intobenVigie:masterfrom
tcoupin:master

Conversation

@tcoupin
Copy link
Copy Markdown

@tcoupin tcoupin commented Mar 19, 2020

No description provided.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

cases: []
};

eval(serverText); // Parse and evaluate js, generate variable gamedata
Copy link
Copy Markdown
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
Copy Markdown
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 :)

}
};

*/
Copy link
Copy Markdown
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...

}


function injectInGameInfoPanel() {
Copy link
Copy Markdown
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
Copy Markdown
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

async function onServerReady() {
console.log('Express server listening on port ' + app.get('port'));

var addresses = getLocalIpAddresses();
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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