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

Implement skins search #274

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

Implement skins search #274

wants to merge 2 commits into from

Conversation

samyycX
Copy link
Contributor

@samyycX samyycX commented May 27, 2023

For #273, @crashzk
This code can enable player to search skins.

Usage

  1. Type !ws <skin name> to search
    like !ws asi
    and it will show a menu like this:
    QQ截图20230527082354
  2. Click the skin you want
    then it will show a menu like this:
    QQ截图20230527082549

Options

  • Apply All: Apply to all weapons that have this skin (with their own skin id)
  • Apply current: Apply to the weapon in player hand with its skin id, if the weapon in player hand doesn't have this skin, it will return SearchApplyCurrentFailed phrase.
  • Specified weapon: this option IS NOT designed for applying the skin to specified weapon
    some skins, like Asiimov, has different skin id for different weapons ( 801 for AK47, 551 for P250)
    this option can apply the skin id you selected to the weapon in your hand (even if the weapon doesn't originally have this skin)
    the reason i designed it like this because i think this will be more fun and useful ( because player can just hold the specified weapon in their hand and click 'Apply current' option instead of setting with this option)

Translation Phrase

  • SearchDisabled
  • SearchApplyAll
  • SearchApplyCurrent
  • SearchApplyCurrentFailed

TODO

since #271 add a cvar sm_weapons_enable_all_skins, we need to do a check before player use the specified weapon options ( or just disable all the specified weapon options )

@crashzk
Copy link
Contributor

crashzk commented May 28, 2023

@samyycX thank you very much, I will be testing this version of yours in the branch I use. Any problems report here, thanks again.

[UPDATE]
I believe you need to make some changes to merge with #271, my version already has it included and gives an error when merging. At least that's what GItHub warns.

ZK-Servidores#7

@crashzk
Copy link
Contributor

crashzk commented May 29, 2023

@samyycX thanks, I'll be testing everything now and come back here. Thanks.

@samyycX
Copy link
Contributor Author

samyycX commented May 29, 2023

@crashzk,

@samyycX thank you very much, I will be testing this version of yours in the branch I use. Any problems report here, thanks again.

[UPDATE] I believe you need to make some changes to merge with #271, my version already has it included and gives an error when merging. At least that's what GItHub warns.

ZK-Servidores#7

Sorry but im not very familiar with git.. Since you have code for PR 271 in your repository, should I commit the code in your repository from now on? Because if I continue to commit the code in my repository, it will be a problem for you to merge

@crashzk
Copy link
Contributor

crashzk commented May 29, 2023

@samyycX my version is now going, I took the liberty of adding the Portuguese-BR translation in my branch.

I believe so, must confirm in my repository. So both your changes and the one in PR #271 take effect if @kgns merges after checking everything.

The only difference between my branch and yours is that I added access to the sm_weapons_enable_all_skins option in the menu for players with VIP. I can change this here easily if needed to merge here.

[UPDATE]
I tested your changes, everything is fine here. I believe I can merge the changes from my branch into yours just to add PT-BR translation and wait for a reply now from the author.

Thanks again, if I get any errors let me know here.

@samyycX
Copy link
Contributor Author

samyycX commented May 29, 2023

@samyycX my version is now going, I took the liberty of adding the Portuguese-BR translation in my branch.

I believe so, must confirm in my repository. So both your changes and the one in PR #271 take effect if @kgns merges after checking everything.

The only difference between my branch and yours is that I added access to the sm_weapons_enable_all_skins option in the menu for players with VIP. I can change this here easily if needed to merge here.

[UPDATE] I tested your changes, everything is fine here. I believe I can merge the changes from my branch into yours just to add PT-BR translation and wait for a reply now from the author.

Thanks again, if I get any errors let me know here.

Sure, my code is basically complete, the only problem is that I haven't make sm_weapons_enable_all_skins cvar control the specified weapons options. since you add the access to it to player, you may modify menus.sp, add a check in SkinsMenuHandler switch case MenuAction_DisplayItem, line 1265 in your repository. You can always modify it and merge.

@crashzk
Copy link
Contributor

crashzk commented May 29, 2023

@samyycX I understand, thank you.

In any case, I saw your PR #274 and #271, both can be merged later without problems, as you mentioned before too, individually.

From what I've seen, they won't generate conflicts if they merge first with 271 and then with 274. My version in this case would be the same as them, but as I mentioned, with VIP access in the sm_weapons_enable_all_skinscvar option. Anyway, that's it for now, after merging both I create a direct PR just to translate the language I use.

And again, thank you!

@samyycX
Copy link
Contributor Author

samyycX commented May 29, 2023

Ok, I will wait for PR #271 to be merged and then make some compatibility changes in this PR afterwards.

@crashzk
Copy link
Contributor

crashzk commented Jun 9, 2023

@kgns sorry for the ping, could this PR #274 and #271 be merged?

#271 should come first in the case so we can see that there won't be compatibility errors with this PR later.

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.

2 participants