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

add aux.SelectPDS and fixed related cards #2541

Closed
wants to merge 3 commits into from
Closed

Conversation

purerosefallen
Copy link
Collaborator

image

fixed this.

@purerosefallen purerosefallen changed the title add Duel.PreserveSelectDeckSequence to 苦渋の選択 add aux.SelectPDS and fixed related cards May 28, 2024
Copy link
Collaborator

@mercury233 mercury233 left a comment

Choose a reason for hiding this comment

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

significantly harder to read

@purerosefallen
Copy link
Collaborator Author

significantly harder to read

I tried something like aux.SelectPreservingDeckSequence but it's a pretty long name

@mercury233
Copy link
Collaborator

#2432 (comment)

@mercury233 mercury233 closed this May 28, 2024
@mercury233 mercury233 deleted the patch-painful branch May 28, 2024 02:56
@purerosefallen
Copy link
Collaborator Author

@mercury233 how about aux.SelectShowDeck?

@mercury233
Copy link
Collaborator

it is obviously unacceptable to add such wrap, please use appropriate method

@mercury233 mercury233 closed this May 28, 2024
@purerosefallen
Copy link
Collaborator Author

it is obviously unacceptable to add such wrap, please use appropriate method

so any better ways?

@mercury233
Copy link
Collaborator

Duel.ShowDeckSequence()
local sg=g:Select(...)
Duel.ShowDeckSequence(false)

nobody will know what is P or what is Preserve

@purerosefallen
Copy link
Collaborator Author

Are you arguing about the naming problem, or you want to have a brand new way to solve this problem?

Please think of ways to solve the problem, rather than closing other's PR or deleting other's branch.

@purerosefallen purerosefallen restored the patch-painful branch May 28, 2024 03:08
@purerosefallen
Copy link
Collaborator Author

Duel.ShowDeckSequence()
local sg=g:Select(...)
Duel.ShowDeckSequence(false)

nobody will know what is P or what is Preserve

Considering the number of cards should be changed, we have to do something on aux.

If you could think of a better name, you could do a direct replacement on those files.

@mercury233
Copy link
Collaborator

Duel.ShowDeckSequence()
local sg=g:Select(...)
Duel.ShowDeckSequence(false)

nobody will know what is P or what is Preserve

Considering the number of cards should be changed, we have to do something on aux.

If you could think of a better name, you could do a direct replacement on those files.

Please change "we" to "I" (you)

@purerosefallen
Copy link
Collaborator Author

Duel.ShowDeckSequence()
local sg=g:Select(...)
Duel.ShowDeckSequence(false)

nobody will know what is P or what is Preserve

Considering the number of cards should be changed, we have to do something on aux.
If you could think of a better name, you could do a direct replacement on those files.

Please change "we" to "I" (you)

Let's consider something real. Which name would you like to use rather than the currenct suffix PDS?

Or if you want another way, could you introduce it?

@mercury233
Copy link
Collaborator

#2432 (comment)

@purerosefallen
Copy link
Collaborator Author

purerosefallen commented May 28, 2024

#2432 (comment)

How about in THIS PR?

I think something like aux.SelectShowDeckSequence but it's still pretty long. Also, I believe we could make a drop-in replacement rather than adding two lines in every card one by one.

@mercury233
Copy link
Collaborator

I think it is better to do something to remove Duel.ConfirmCards from those effects and close Fluorohydride/ygopro#2470
Anyway you shouldn't (and can't) add a wrap version to every select function, and aux.selpds will be even better than aux.SelectPDS because the latter looks like a new type of abbreviation

@purerosefallen
Copy link
Collaborator Author

I think it is better to do something to remove Duel.ConfirmCards from those effects and close Fluorohydride/ygopro#2470 Anyway you shouldn't (and can't) add a wrap version to every select function, and aux.selpds will be even better than aux.SelectPDS because the latter looks like a new type of abbreviation

  1. Previous time I asked if you would merge update MSG_CONFIRM_CARDS, add skip_panel ygopro#2470 on the incoming release, but I got a reply like no time to do ygopro2.
  2. This function is only needed to select cards from deck when it's on deck top or bottom, or opponent selecting deck cards, being a rare case. In addition, the idea prior to use dummy in-deck sequence for MSG_SELECT_* ygopro-core#564 which is adding an aux.SelectDeckCard you also supported the idea to wrap every select method, and that one is a much wider case. So what exactly is your idea on wrapping select method?
  3. Or should we do them in the core way instead? I'm thinking of hiding deck sequence in selecting player's own decks only, showing deck sequence as usual when selecting opponent's deck.

@purerosefallen
Copy link
Collaborator Author

@purerosefallen purerosefallen deleted the patch-painful branch May 28, 2024 03:50
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