Skip to content

Conversation

@guilherme-gm
Copy link
Member

Pull Request Prelude

Changes Proposed

This PR implements script commands to identify items, although lots of scripts nowadays kind of fake that behavior by using a combination of delitem2 + getitem2, this leads to wrong behavior when items have more information, like random options (as shown in this forum post, where doing this leads to losing dropped options). New script commands:

  • identify(<Item ID>): Identifies the first unidentified item with ID = "Item ID"
  • identifyidx(<idx>): Identifies item at inventory index idx.

I took the chance to also add @identifyall atcommand, because it is quite boring to identify item per item when you are testing lots of drops.

Issues addressed:

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@guilherme-gm guilherme-gm changed the base branch from stable to master June 2, 2019 02:25
@guilherme-gm guilherme-gm changed the title Add identify script commands and identifyall atcommand Add identify/identifyidx script commands and identifyall atcommand Jun 2, 2019
for (int i = 0; i < sd->status.inventorySize; i++) {
if(sd->status.inventory[i].nameid > 0 && sd->status.inventory[i].identify!=1){
if (sd->status.inventory[i].nameid > 0 && sd->status.inventory[i].identify != 1) {
if (identifyall)
Copy link
Member

Choose a reason for hiding this comment

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

perhaps move this line to before the for-loop? so that it won't need to go through the loop everytime even if we are just using @identify to identify one item.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering that too, but it would duplicate most of the loop code so I wasn't sure if I should

Copy link
Member

@dastgirp dastgirp Jun 2, 2019

Choose a reason for hiding this comment

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

@Emistry it needs to loop even if you use @identify, to know how many item needs to be identified.
That was the old behavior too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is as dastgir said. Can I mark as resolved and leave it as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

for faster code if (identifyall) better move outside of for
this mean for faster code need two for one inside if block and second inside else block

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I'm wondering, would make more sense to make a separate command block instead of using the same block + strcmp?

@Emistry Emistry added component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder labels Jun 2, 2019
@MishimaHaruna
Copy link
Member

@guilherme-gm I force-pushed on your branch a minute after you did (without realizing) - if you had made any changes (or just to restore your gpg signature, if any), please force-push again, otherwise I'll merge this as is

@guilherme-gm
Copy link
Member Author

@MishimaHaruna no problem :) you can merge it as is

@MishimaHaruna MishimaHaruna merged commit 3fe9300 into HerculesWS:master Jun 30, 2019
@guilherme-gm guilherme-gm deleted the 201906-1-identify branch February 24, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants