-
Notifications
You must be signed in to change notification settings - Fork 90
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
[Fight For Style] Ajout d'un Show Room et la création de vetement sous forme d'item. #128
base: oss
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deux petits comment sur des changements plus ou moins relatif au PR
public applyComponent(component: Component, outfitItem: OutfitItem) { | ||
public applyComponentToPed(ped_id: number, component: Component, outfitItem: OutfitItem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quelque amélioration des fonctions de base afin de pouvoir les utilisés sur un PED autre que le ped joueur
ped.target.options | ||
ped.target.options, | ||
ped.target.distance || undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Petit fix pour la function createForPed
qui ne transferai pas correctement ped.target.distance
à createForBoxZone
. Ce qui faisant qu'en utilisant createForPed
avec une distance custom, la distance par defaut (2.5) était utilisé
Voici un lien avec la démo final (feature complète): https://drive.google.com/file/d/1JO_VpWme5-k-lNb0uHTv11rpzesVL3PO/view |
Je viens de push les changement demander, voici la vidéo qui va avec: |
f815f8b
to
5a46d12
Compare
Le dernier commit fix les soucis avec les actions github (eslint) |
655cea7
to
8b90290
Compare
1aeb82b
to
7cf3f12
Compare
Branch rebased, conflict fixed. |
7cf3f12
to
fbb4513
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
Just to mention, I will open another PR tonight to prepare potential clothes exclusion from the Show Room (in case there is some police/undesirable hat/clothes in the remaining categories), so I will just need to add them to the list based on report. |
@@ -0,0 +1,434 @@ | |||
import { Once, OnceStep, OnEvent, OnNuiEvent } from '../../../core/decorators/event'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mettre .provider
dans le nom du fichier
|
||
private readonly CAMERA_POSITION: Vector3 = [-166.93, -300.1, 40.13]; | ||
|
||
private ped_id = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camelCase
|
||
private ped_id = null; | ||
|
||
private outfit_type = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camelCase
(et les autres en dessous aussi)
|
||
private readonly CAMERA_TARGET: Vector3 = [-168.39, -298.7, 39.83]; | ||
|
||
private readonly CAMERA_POSITION: Vector3 = [-166.93, -300.1, 40.13]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tu peux les mettre en constantes en haut
|
||
private outfit_type = null; | ||
|
||
private components_to_craft: object = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: Record<Component, OutfitItem>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus besoin, la propriété a été retirée
}); | ||
|
||
return Ok(true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revoir tout le fonctionnement, pas besoin d'état dans ce provider, tout peur être stocké coté NUI dans les composants via un useState
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai supprimer quasiment tout sauf le pedId
sur le provider
maxDrawables: number; | ||
}[]; | ||
}; | ||
can_craft: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camelCase 🙏
export const FightForStyleShowRoomMenu: FunctionComponent<FightForStyleShowRoomComponent> = ({ data }) => { | ||
const banner = 'https://nui-img/soz/menu_job_ffs'; | ||
const [currentDrawable, setCurrentDrawable] = useState<number>(0); | ||
const [currentCraft, setcurrentCraft] = useState<{ drawables: object; props: object }>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attention au nom setCurrentCraft
private inventoryManager: InventoryManager; | ||
|
||
public async useOutFit(source: number, item: CommonItem, inventoryItem: InventoryItem) { | ||
const player = this.playerService.getPlayer(source); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
faire un check si player null (ca reste une possibilité - rare)
async onShowRoomCrafting(source: number, outfit: object, outfit_type: string) { | ||
const player = this.qbcore.getPlayer(source); | ||
|
||
let cost_mult = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camelCase
@@ -159,6 +159,10 @@ export type InventoryItemMetadata = { | |||
tier?: number; | |||
crafted?: boolean; | |||
id?: string; | |||
// FFS Showroom | |||
drowables?: object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drawables ?
Pour la liste des vetements il faudrait faire une liste qu'on autorise et pas en mode exclusion, éviter les problèmes a chaque nouveau dlc. |
Fix ce qui a été demander. Il se fait tard, je m'occuperai demain du système d'inclusion ✅ |
Progress du filtrage: Components: Props: Je vais mettre à jour ce commentaire avec ma progression, et j'en posterai un autre quand tout sera dans le code et également pour dire ce qui a été retiré. J'inclurai ma doc et tout ce qu'il faut pour le modifier dans le future au besoin. Je pense que cela sera fini en fin de semaine ✔️ Résumé de ce qui est filtré: Chapeau: Damier, vide, casque d'avion de chasse, Casquette securoserv, casque avec douille, lunette infrarouge, casquette paramedit, casque de demineur, casque de pompier, casquette de céréminie (FDO), chapeau FDO/service publique |
Tri terminé, jolie GDOC de 4074 ligne. |
2c1e271
to
5b6ca16
Compare
Changement effectué! Tout est bon pour la re-review. Ici les changements apportées:
Voici une petit vidéo avec les changements: https://drive.google.com/file/d/14u1zpjP9bY5Jf2oeVi5drhT6Avmg_lnA/view |
@@ -2817,6 +2817,30 @@ QBShared.Items = { | |||
['combinable'] = nil, | |||
['description'] = 'L\'outil parfait pour faire infecter un ordinateur' | |||
}, | |||
['ffs_crafted_outfit_m'] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
il n'y a pas les nouvelles images ? on peut les mettre dans le dossier soz-core/public/images/items en png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pas de nouvelle image à ce moment, je suis pas très bon avec Mid Journey, mais je peux essayer ça
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -26,6 +26,7 @@ | |||
"chokidar": "^3.5.3", | |||
"concurrently": "^7.2.2", | |||
"css-loader": "^6.7.1", | |||
"csvtojson": "^2.0.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
j'ai vu le csv plus bas, pas sur que ce soit le meilleur move, on peut pas avoir directement le json ? quitte a avoir l'outil a coté pour transformé tout cela ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, c'est possible oui. J'ai tout rajouté dedans pour éviter que ce soit perdu dans le temps. Au besoin, je peux rajouter un commentaire dans le script de conversion.
Du coup, le CSV en soit peut être retirer du repo, par contre tu penses quoi de garder le script de conversion?
Il faudrait aussi rajouter le CSV dans un document interne, pour éviter d'avoir à le refaire encore une fois si besoin de l'éditer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dans l'idée, avoir le CSV dans le repos permet à ceux qui utilise l'OSS d'avoir la base du travail avec le bon format. Sans ça, il ne pourront pas faire de modif sans reverse le script de conversion. Du coup c'est un peu un choix à faire.
Je te laisse me dire ce que tu préfère.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Un JSON directement me semble bien trop complexe, chiant à éditer car si on veut le changer, il faut le convertir dans un sens en CSV pour l'édit PUIS le reconvertir en JSON par la suite, vu le nombre de data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En fait je vois que c'est en dev est deja transformé plus bas avec le fichier conversion.js on peut rajouter la commande dans le package.json ?
On pourrait aussi avoir le fichier autre part que dans src non ? (Genre un dossier data a la racine du core ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En fait je vois que c'est en dev est deja transformé plus bas avec le fichier conversion.js on peut rajouter la commande dans le package.json ?
On pourrait aussi avoir le fichier autre part que dans src non ? (Genre un dossier data a la racine du core ?)
Je vois ce que tu veux dire, je m'en occupe de suite 🙏
4886432
to
bb46304
Compare
Changement effectuer, ajout d'une commande dans le package.json, modification de l'emplacement du script et du fichier CSV dans un dossier data 🙏 En esperant avoir bien compris ta demande Pour les images, j'ai fait les demandes sur le discord Midjourney, elles pourront au besoin être ajouter plus tard ✔️ J'ai également rebase à nouveau pour régler les conflits |
Je rajoute les images dans la nuit, on a pu m'en générer! Et j'ajouterai le nom discord de la personne directement dans l'objet. Je rajouterai un commentaire quand ce sera fait |
Ca devrait être bon pour une re-review. Tout ce qui a été demandé a été ajouter/modifier 🙏 |
136d789
to
922ca8a
Compare
Rebase et squash les changement demander en un commit (le deuxième) |
922ca8a
to
bb01d57
Compare
Rebase fait à nouveau ce matin après les ajout de la nuit 🏁 |
bb01d57
to
a530fe3
Compare
Heu.. erreur de manip. Je push ça à nouveau rapidement |
C'est bon, réouvert avec rebase 🙏 |
Je vais rebase d'ici ce week end avec les gros changement qu'il y a eu et fix au besoin certaine partie |
b40036d
to
6146179
Compare
Rebase à ce jour, pas de conflit. |
Le fail des test auto ne viennent pas de la PR, du coup toujours good à rereview |
Bonjour. J'ai pour projet de reprendre cette PR, si elle intéresse toujours. Mais vu le taff qu'il y a à faire au niveau du rebase, je voulais m'assurer que cela intéressait toujours dans un premier temps? Merci bien 🙏 |
Oui tu peux ! |
Parfait, j'essaye d'avoir le rebase fait d'ici deux semaines |
ddc08cd
to
8bfc186
Compare
La PR a totalement été rebase (enfin, je suis repartie de 0 et j'ai tout réintégré dans le code mise à jour). |
3a1b750
to
fbd48fd
Compare
Coucou, ce PR fait suite à l'issue #73
Voici ce que cela la feature ajoute:
J'ai fait également quelque amélioration pour éviter de dupliquer le code, que je vais commenter par la suite.
Je reste dispo pour toute modification que vous pensez nécéssaire 🙏 En espérant que la feature vous plaira!