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

flag (no interact) quest GIVER Game Object when player not eligible for that quest #157

Closed
wants to merge 2 commits into from

Conversation

lduguid
Copy link

@lduguid lduguid commented Jun 18, 2016

Using GAMEOBJECT_DYN_FLAGS will flag quest giver Game Object (GO) GO_DYNFLAG_LO_NO_INTERACT if the player is not eligible for the quest. Lack of eligibility is determined by the player not being active to quest and there being no quest text to display.

This was motivated by the need to suppress the default greeting text on various quest giver game object such as wanted posters when the player clicked on these items but was not eligible for that quest or that part of a quest chain etc.

See WANTED Poster Greets You #142 and Gadgetzan #860 for more history and details motivating this pull request.

*data << uint16(0);
}
else
*data << uint32(0); // disable quest object
Copy link
Contributor

Choose a reason for hiding this comment

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

Very small thing but should move this comment to line 520 as it applies to the whole else statement.

@@ -517,8 +517,18 @@ void Object::BuildValuesUpdate(uint8 updatetype, ByteBuffer* data, UpdateMask* u
break;
}
}
else
*data << uint32(0); // disable quest object
else // flag (no interact) quest object when player not eligible for quest
Copy link
Author

Choose a reason for hiding this comment

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

as requested, move comment up to else as it applies to the whole else statement.

@lduguid
Copy link
Author

lduguid commented Jun 18, 2016

Although this fix appears to take care of most of the quest giver objects, such as wanted posters and the Conspicuous Gravestone in Gadgetzan etc. It should be noted that quests like the 'Old Lion Statue' (http://db.vanillagaming.org/?object=31), where the player interacts with this quest giver GO, and a progress bar (like opening a chest) is activated and then once the progress is up the quest dialog/text is displayed, even after this change the default greeting message is still displayed.

Perhaps this is correct behavior and that quest just needs more appropriate quest text when the player is not eligible for the quest yet? However if this is not the case, the interaction should be flagged here as well. It might require doing somewhere else as it does not appear to be handled at Object.cpp, Object::BuildValuesUpdate(...) where the other quest giver game objects are now flagged.

For the 'Old Lion Statue' , the quest game object the logic that handles showing the quest text is rather in SpellEffects.cpp, Spell::EffectOpenLock(...)-> Spell::SendLoot(...)., I am not sure how we can flag it appropriate or if it is even possible here? or as said earlier, it could be correct behavior now and more appropriate quest text is required?

@lduguid lduguid changed the title flag (no interact) questGO when player not eligible for that quest flag (no interact) quest giver GO when player not eligible for that quest Jun 20, 2016
@killerwife
Copy link
Contributor

I think it should have "untargetable" flag in DB, which would make it targetable only when on the quest?

@lduguid
Copy link
Author

lduguid commented Jun 22, 2016

Not sure if we make quest GIVER game objects 'untargetable'? Is no interact and untargetable going to have the same effect? quest game objects/items Vs. quest giver game objects appear to be managed subtly different from what little I was able to determine when looking into this pull request but it is most likely I missed the big picture in how it all work together.

For example, if the quest giver object was untargetable instead of no-interact, would that mean the player can still mouseover the wanted posted and see information on it etc. ? If it solves the problem and works in better with the existing way quest giver objects work I am all for it =) I just have no idea how to go about it. I do mind rolling my sleeves up and trying it out but I might need a few hints on where to dig deeper etc.

@killerwife
Copy link
Contributor

Yes, we definitely make quest GOs untargetable, I was fiddling around with it yesterday. But that is not to say there cant be an exception. (example is chest in lakeshire in the lake is untargetable when not on quest)

@lduguid
Copy link
Author

lduguid commented Jun 22, 2016

Was that the quest for Oslow's Toolbox on the The Lost Tools quest where your find the Sunken Chest ? if so, although, I maybe being overly pedantic on terminology, these are different, and from what I can gathered, handled differently, in that the the sunken locker is a quest game object/item, where as say a wanted poster is a quest GIVER game object. i.e GAMEOBJECT_TYPE_QUESTGIVER Vs. GAMEOBJECT_TYPE_CHEST (which does not always have to be a literal chest e.g. bingles toolbucket etc.).

This may not matter, in that we treat them the same regardless? I cannot recall how vanilla retail treated say a wanted poster which is expected to prompt the player with quest message when appropriate versus a quest game object that completes a quest or makes quest loot available when clicked, if appropriate.

@killerwife
Copy link
Contributor

Hmmm, I see where you are coming from. Well a wanted poster should only be empty, but if it were a statue, I would say it shouldnt be "targetable" if it doesnt have gossip_menu text

@lduguid lduguid changed the title flag (no interact) quest giver GO when player not eligible for that quest flag (no interact) quest GIVER Game Object when player not eligible for that quest Jun 22, 2016
@lduguid
Copy link
Author

lduguid commented Jun 22, 2016

OK. I will fiddle around to see if I can make cases like the old lion statue behave more like a traditional quest game object and become untargetable when there is no gossip menu text. As you suggest that would more likely be a database flag change. Currently I still believe this particular pull request stands for quest GIVER game objects unless i can figure out a way for them to also all be handled appropriately with db flagging.

@killerwife
Copy link
Contributor

Sure, I just suggested what to investigate :)

@cala
Copy link
Contributor

cala commented Jun 23, 2016

Assigned myself. I'll try to review this next week. Thanks.

@cala cala self-assigned this Jun 23, 2016
@cala
Copy link
Contributor

cala commented Jul 2, 2016

Is there any prerequisite DB-side before testing this PR? In its current state, players can still interact with quest GOs even not on the related quest(s) (or I missed something while/before testing).

@lduguid
Copy link
Author

lduguid commented Jul 2, 2016

There is no DB-side for the pull request.

In an nutshell, as it stands now, all it does is suppress the 'Greetings playername quest dialog box if the player clicks on a quest GIVER GO (e.g. Wanted Poster's/tombstone) and they are not applicable for that particular quest.

As stated above, this is different to a quest GO, a quest GO does not allow any interaction or mouse-over feedback unless the player is on the relevant quest and/or quest chain. For example, a wanted poster, which is quest GIVER GO, will still give some feedback (e.g. 'Wanted blah...' if you mouse over it, even if you are not applicable for the quest but will not present you with the quest dialog until you are applicable.

Having said all that, it should noted that this may not be the desired behavior and that we want to treat quest GIVER objects the same as quest GO's, that is no interaction/mouseover feeback etc. at all if the user is not applicable for that quest pickup. I believe that is how it is currently handled on Kronos 2 but my memory of how it really was on retail classic is hazy at best.

I do know of at least one Quest giver Go that will still present you with a greetings dialog after applying this pull request and that is the 'old lion statue' in Redridge. see previous explanations of why that maybe so... If during your testing after applying this pull request you are still seeing quest giver objects showing the default greetings dialog, let me know which ones they are and I will take a look at it.

@cala
Copy link
Contributor

cala commented Jul 2, 2016

In an nutshell, as it stands now, all it does is suppress the 'Greetings playername quest dialog box if the player clicks on a quest GIVER GO (e.g. Wanted Poster's/tombstone) and they are not applicable for that particular quest

Thanks for clarifying. That's what I understood at first and unless I missed something, the tombstone near Gadgetzan and the Wanted poster in Thelsamar are still displaying the "Greetings $n" default text. Hence, my question. I'll try to build it anew.

@Phatcat
Copy link
Contributor

Phatcat commented Jul 2, 2016

Having said all that, it should noted that this may not be the desired behavior and that we want to treat quest GIVER objects the same as quest GO's, that is no interaction/mouseover feeback etc.

*Edit: quoted the wrong part of your message there, sorry.

I think this is WRONG. My motivation for this trail of thought is that such a change would make players unable to hover the mouse over quest giver objects where the player is not high enough level to start the quest, yet the player will still see the silver exclamation mark. Those 2 things won't ever go well together if you ask me.

@lduguid
Copy link
Author

lduguid commented Jul 2, 2016

I was probably not clear and unfortunately the terminology does get overly pedantic with quest GO's versus quest giver GO's but I think we are on the same page as this pull request should allow the user to mouse-over quest giver game objects at all times (levels, not on quest etc.) to get feedback on what the quest is, if it is working properly. The only thing it should suppress, is the quest default 'Greeting's palyername' if the player is not applicable.

@Phatcat
Copy link
Contributor

Phatcat commented Jul 2, 2016

I know this Pr does not include the change you mentioned which I quoted, but you very much pondered the change, and so I thought I would voice my concerns with that; and only that. This PR itself 👍

@lduguid
Copy link
Author

lduguid commented Jul 2, 2016

I am not attached to this way of doing it, if someone else has a clear recollection of how the player interacted with the various different kinds of game objects (quest, quest giver, chest opening etc. ) and if they were treated all the same or how the deviated in their interaction, let us know and I will be glad to try and implement it that way =).

If nothing else, this hopefully gets us thinking how the system should more closely react like classic retail in these circumstances, besides the current behavior of a presenting a quest dialog with a default greeting's playername message. I'm all for whatever works best.

@lduguid
Copy link
Author

lduguid commented Jul 3, 2016

@cala how did it go after a new build, were you able to see the desired result or are you still seeing the old behavior?

@cala
Copy link
Contributor

cala commented Jul 3, 2016

how did it go after a new build, were you able to see the desired result or are you still seeing the old behavior?

Still having the old behaviour with GO 148504 (A Conspicuous Gravestone) and 185166 (Wanted Poster). I tested a few more and it was the same. Weird. :/

@lduguid
Copy link
Author

lduguid commented Jul 3, 2016

That is strange.

I am pretty sure my cmangos-classic source code wise, was just the latest build when the pr was made with just the pr changes applied, however, I cannot speak for the how pristine my database was at the time, although it should not have deviated too much from a stock standard one. Having said that, even if there were 'custom' changes in my db, it should not have influenced this pr unless there was quest text message for the quest but if that was the case you would not get the default greeting message anyway.

I will get a clean build of everything myself to be sure, test it without and then with pr and see if there is something else going on.

@lduguid
Copy link
Author

lduguid commented Jul 3, 2016

Argh!! Like you I am now witnessing the old behavior of the default greeting message quest dialog even after I apply this PR to the latest and greatest (code and database) but if i go back to an older copy (when I made the PR) it works as desired. Unfortunately the older copy I am testing with is just a binary backup with the PR applied.

For now it is best to stop testing on this and put it on hold until I figure out what has changed and if it is still worth pursuing. I guess i could start git bisecting from that earlier build to now and see what may have changed that could possibly influence this, it is definitely nothing to do with the immediate code changes as Object.cpp has not changed in months. It does beg the question that if it it some 'unrelated' change that can influence this behavior with the PR applied, this PR change might have been a too brittle a modification to make in the first place with the potential to break again in the future regardless of what I find. I will keep you updated if I discover anything.

Sorry for the inconvenience. Its one of those 'but it works on my machine...' ;)

@cala
Copy link
Contributor

cala commented Jul 3, 2016

No worries! GO testing is always a bit tricky because it seems that their cache is not always cleaned up as it should. I wanted to be sure that that was not the case for me.

I let you look if you can find what has changed and what was previously working. Thank you for your PR and all the explanations/testing that went with it anyway. 😄

@lduguid
Copy link
Author

lduguid commented Jul 5, 2016

Well the plot thickens but for now I will just say that the primary problem with this pull request implementation was between my chair and keyboard. In a nutshell i messed up snipping between multiple repositories I maintain and was not being over diligent and testing across all of them. My apologies. I have another idea on how to achieve the same desired results. I am pressed for time at the moment but will endeavor to submit another pull request on the weekend, I will leave this one open for a reminder until I submit the new one.

@Phatcat
Copy link
Contributor

Phatcat commented Jul 5, 2016

the plot thickens

😄

You don't have to open a new PR, you can fix this one simply using git; #119 (comment)

@Phatcat
Copy link
Contributor

Phatcat commented Aug 12, 2016

Looking forward to this still, hoping you havn't given up on it.

@lduguid
Copy link
Author

lduguid commented Aug 12, 2016

Not given up on but had definitely relegated to the back burner... I will take a look again on the weekend and submit what I had working, although it did not utilize the previously suggested GO_DYNFLAG_LO_NO_INTERACT, I was never able to get that to work. Maybe someone else will chip in if I submit a PR not using it and can show how to use GO_DYNFLAG_LO_NO_INTERACT correctly.

Thanks for the reminder. =)

@lduguid
Copy link
Author

lduguid commented Aug 16, 2016

Work is consuming my time, blah blah blah... Long story short, I will try again this weekend, apologies for the delay.

@cala
Copy link
Contributor

cala commented Aug 16, 2016

No hurry, no worries. Take the time you need to work on this with a fresh and clean mind rather than doing it in a rush or in unmotivated state of mind. Remember: we are doing this for fun. 😉

Thank you for the update anyway.

@Gerhood
Copy link

Gerhood commented Aug 16, 2016

After I finally created a Trinity PR for this some new information surfaced: TrinityCore/TrinityCore#17791 you should definitely take a look!

@cyberium
Copy link
Member

@cala i think we can close this as it seem a simple db could fix that. TrinityCore/TrinityCore#17803

@lduguid lduguid closed this Dec 20, 2016
@lduguid lduguid deleted the flag_questGO_nointeract branch December 20, 2016 21:42
@cala
Copy link
Contributor

cala commented Dec 21, 2016

@cyberium I test the SQL query in the link you provided and it did not fix the issue.
We need to check if our core handles those flags the same way TC core does.

@cyberium
Copy link
Member

cyberium commented Dec 22, 2016

Ok so it may require some core part.

Could you confirm that the request suggested is correct? I mean can i use the flag suggested by that request in the core?

Edit: just tryed this on tbc and classic and it worked for the gadjetzan graveyard

-- on tbc
mysql> UPDATE gameobject_template SET flags = flags | 4 WHERE type = 2 AND Data3 = 0;
Query OK, 314 rows affected
Rows matched: 361  Changed: 314  Warnings: 0

-- on classic
mysql> UPDATE gameobject_template SET flags = flags | 4 WHERE type = 2 AND Data3 = 0;
Query OK, 259 rows affected
Rows matched: 263  Changed: 259  Warnings: 0

on tbc
gadgetzan graveyard
on classic
gadgetzan graveyard

Ah, i may need to check now if it correctly actived if the condition is fullfiled.

@Grz3s
Copy link
Member

Grz3s commented Jan 4, 2017

hi
@cyberium ask me ... to look at this issue...
So:
First it doesnt happen in wotlk
Basicaly object u talking about have Flags = 4.
I'll give you smth that will proof what im sain.
.. Lakeshire:
Gameobject = 47 "Wanted: Lieutenant Fangore" - is a questgiver.
Player takes quest ... and is not able to target it anymore:
Luckly i have some old wow guide vids .. to proof this:
https://www.youtube.com/watch?v=OgLha6PPsog 7:27 (player takes quest from that item)
and on this vid:
https://www.youtube.com/watch?v=ds5ZOvNoqRY (1:30 - 1:40) player few Times moves his cursor over that object... and nothing happens (no interaction : object should be highlighted or yellow name should show up on the bottom of screen)
This is how it also works on WoTLKDB.

Also... i looked on old sniffs: i have one from 3.3.0

[3] UpdateType: CreateObject1
[3] GUID: Full: 0xF11000002F0004EC Type: GameObject Entry: 47 Low: 1260
[3] Object Type: GameObject (5)
[3] Update Flags: Unknown1, LowGuid, StationaryObject, GOPosition, GORotation (856)
[3] GO Transport GUID: 0x0
[3] GO Position: X: -9247.361 Y: -2151.597 Z: 63.93343
[3] GO Orientation: 3.141593
[3] GO Transport Position: X: -9247.361 Y: -2151.597 Z: 63.93343 O: 3.141593
[3] Corpse Orientation: 0
[3] Unk Int32: 3746617933
[3] Low GUID: 788530412
[3] GO Rotation: X: 0 Y: 0 Z: -1 W: 0
[3] OBJECT_FIELD_GUID: 788530412/1.164328E-10
[3] 1: 4044357632/-7.130535E+29
[3] OBJECT_FIELD_TYPE: 33/4.624285E-44
[3] OBJECT_FIELD_ENTRY: 47/6.586103E-44
[3] OBJECT_FIELD_SCALE_X: 1065353216/1
[3] GAMEOBJECT_DISPLAYID: 17/2.382207E-44
[3] GAMEOBJECT_FLAGS: 4/5.605194E-45             <---- this should be enough :)
[3] GAMEOBJECT_PARENTROTATION + 3: 1065353216/1
[3] GAMEOBJECT_DYNAMIC: 4294901760/NaN
[3] GAMEOBJECT_FACTION: 12/1.681558E-44
[3] GAMEOBJECT_BYTES_1: 4278190593/-1.701516E+38

I also checked that grave and poster in tanaris...
Same situation.

@killerwife
Copy link
Contributor

If it does not happen in wotlk I would seriously suggest checking code handling classic vs wotlk.

@Tobschinski
Copy link
Contributor

Cyberium just made me test it and I cannot report any issues with it, was working fine.

@cala
Copy link
Contributor

cala commented Jan 6, 2017

So, if nobody still get this issue, we can safely check it off our list, then.

@killerwife
Copy link
Contributor

Not occuring on my end after setting flag.

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.

8 participants