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

Blink v4 #15796

Closed
wants to merge 1 commit into from

Conversation

@kvipka
Copy link

kvipka commented Nov 1, 2015

  • blink in abyss
  • blink by path ( rechecking each step )
  • where player can walk - blink will available there
  • implemented check on dynamic objects (locked doors and etc)
  • implemented total path of blink.

video : http://www.youtube.com/watch?v=wpymzrkwNTg

@msoky

This comment has been minimized.

Copy link
Contributor

msoky commented Nov 1, 2015

👍

@FrancescoBorzi

This comment has been minimized.

Copy link
Member

FrancescoBorzi commented Nov 1, 2015

@kvipka good job,

please fix that line pointed by @Naios and squash your commits

@ipanamski

This comment has been minimized.

Copy link

ipanamski commented Nov 1, 2015

Nice!

@WoWShards

This comment has been minimized.

Copy link

WoWShards commented Nov 2, 2015

give this man a cookie! one of best PRs.

@Takenbacon

This comment has been minimized.

Copy link
Contributor

Takenbacon commented Nov 4, 2015

The amount of calls to vmaps makes me cringe.

float overdistance = 0.0f;
float totalpath = 0.0f;
float beforewaterz = 0.0f;
const float step = 2.0f;

This comment has been minimized.

Copy link
@Naios

Naios Nov 4, 2015

Contributor

Trinity uses the

float const name

syntax in most cases.

if (!m_caster->HasUnitMovementFlag(MOVEMENTFLAG_FALLING) || (z - ground < 25.0f))
{
float tstX, tstY, tstZ, prevX, prevY, prevZ;
float tstZ1, tstZ2, tstZ3, destz1, destz2, destz3, srange, srange1, srange2, srange3;

This comment has been minimized.

Copy link
@Naios

Naios Nov 4, 2015

Contributor

Prefer arrays over variables named like:
name1, name2, name3

//distance of rays, will select the shortest in 3D
srange1 = sqrt((tstY - prevY)*(tstY - prevY) + (tstX - prevX)*(tstX - prevX) + (tstZ1 - prevZ)*(tstZ1 - prevZ));
srange2 = sqrt((tstY - prevY)*(tstY - prevY) + (tstX - prevX)*(tstX - prevX) + (tstZ2 - prevZ)*(tstZ2 - prevZ));
srange3 = sqrt((tstY - prevY)*(tstY - prevY) + (tstX - prevX)*(tstX - prevX) + (tstZ3 - prevZ)*(tstZ3 - prevZ));

This comment has been minimized.

Copy link
@Naios

Naios Nov 4, 2015

Contributor

Related to my comment above, if you would use arrays you could save a lot of code here.


dest = SpellDestination(destx, desty, z, orientation);
//float range = sqrt((desty - y)*(desty - y) + (destx - x)*(destx - x));
//TC_LOG_ERROR("server", "Blink number 2, in falling but at a hight, distance of blink = %f", range);

This comment has been minimized.

Copy link
@Naios

Naios Nov 4, 2015

Contributor

Do you plan to enable this log messages? Otherwise remove it please.

This comment has been minimized.

Copy link
@WoWShards

WoWShards Nov 4, 2015

I guess yes, he is, else he would not doing it.

This comment has been minimized.

Copy link
@Shauren

Shauren Nov 4, 2015

Member

FYI if you need debug prints VS allows you to print custom messages with breakpoints (without stopping the application)


if (!m_caster->HasUnitMovementFlag(MOVEMENTFLAG_FALLING) || (z - ground < 25.0f))
{
float tstX, tstY, tstZ, prevX, prevY, prevZ;

This comment has been minimized.

Copy link
@Naios

Naios Nov 4, 2015

Contributor

C++ doesn't support variable hoisting like JavaScript does, that's why we declare the variables at first use in most cases and not at the top of the method.

This comment has been minimized.

Copy link
@WoWShards

WoWShards Nov 4, 2015

I see you are a expert in this 👍 glad to have you "schön dich mit dabei zu haben"

@wrathix

This comment has been minimized.

Copy link

wrathix commented Nov 4, 2015

The video looks like it works amazing! Well done

destz3 = map->GetHeight(phasemask, destx, desty, prevZ - maxtravelDistZ / 2, true, 10.0f);

//distance of rays, will select the shortest in 3D
srange1 = sqrt((desty - prevY)*(desty - prevY) + (destx - prevX)*(destx - prevX) + (destz1 - prevZ)*(destz1 - prevZ));

This comment has been minimized.

Copy link
@Naios

Naios Nov 4, 2015

Contributor

Prefer G3D::Vector over self made solutions:

G3D::Vector3 dest, prev = .... ;

auto diff = dest - prev;

float range = diff.length();

This comment has been minimized.

Copy link
@Shauren

Shauren Nov 4, 2015

Member

or Postion::GetExactDist...

This comment has been minimized.

Copy link
@Asterc

Asterc Jan 5, 2016

Contributor

could you guys show how to fix the rest of the codestyle? I assume most people here are not programmers, and find it confusing.
I changed some of the float to Position and other things here
https://gist.github.com/Asterc/c73e779ab52649f793dd
not sure about how to proceed with the rest

srange2 = sqrt((desty - prevY)*(desty - prevY) + (destx - prevX)*(destx - prevX) + (destz2 - prevZ)*(destz2 - prevZ));
srange3 = sqrt((desty - prevY)*(desty - prevY) + (destx - prevX)*(destx - prevX) + (destz3 - prevZ)*(destz3 - prevZ));

if (srange1 < srange2)

This comment has been minimized.

Copy link
@Naios

Naios Nov 4, 2015

Contributor

If you use the array recommended above you could use:

destz = *std::min_element(std::begin(array), std::end(array));

This comment has been minimized.

Copy link
@Naios

Naios Jan 10, 2016

Contributor

All in all this if conditions don't select the shortest range... consider: srange1 = 2, srange2 = 3, srange3 = 1... the branch will select 2 instead of 1.

@kvipka

This comment has been minimized.

Copy link
Author

kvipka commented Nov 4, 2015

Okay, i added Position, but Core has a trouble in relocate func (i think)
Look on this video:
https://www.youtube.com/watch?v=qLGG_aI14hA

Light has a right coords, but player will "nearteleport" on his old position.

Need investigation and help, @Shauren

// we have correct destz now
}
//}
Position lastpos;

This comment has been minimized.

Copy link
@Naios

Naios Nov 4, 2015

Contributor
Position lastpos = {destx, desty, destz + 0.5f, pos.GetOrientation()};

You could introduce Position earlier when using map->GetHeight

@kvipka

This comment has been minimized.

Copy link
Author

kvipka commented Nov 18, 2015

After tests of players, founded 3 trouble sutuations, fixed:
https://www.youtube.com/watch?v=8UCnK7BNTUM

@FrancescoBorzi

This comment has been minimized.

Copy link
Member

FrancescoBorzi commented Nov 19, 2015

@kvipka please can you put srange1, srange2 and srange3 into an array srange and replace the if-else with:

destz = *std::min_element(std::begin(srange), std::end(srange));

as @Naios suggested

if (!m_caster->HasUnitMovementFlag(MOVEMENTFLAG_FALLING) || (pos.GetPositionZ() - ground < 25.0f))
{
float tstX, tstY, tstZ, prevX, prevY, prevZ;
float tstZ1, tstZ2, tstZ3, destz1, destz2, destz3, srange, srange1, srange2, srange3;

This comment has been minimized.

Copy link
@Naios

Naios Nov 19, 2015

Contributor

Convert this to Position or G3D::Vector3 please.

This comment has been minimized.

Copy link
@Asterc

Asterc Jan 5, 2016

Contributor

This line only deals with Z positions, is ok to use Position or G3D :: Vector3 anyway?

This comment has been minimized.

Copy link
@r00ty-tc

r00ty-tc Feb 29, 2016

Contributor

tstX/Y/Z and prevX/Y/Z could stand to be made Position. Since later, where there's currently a range calculation made in the code, instead the Position.GetExactDist2d could be used.

The Z's should remain floats.

tstZ3 = map->GetHeight(phasemask, tstX, tstY, prevZ - maxtravelDistZ / 2, true, 25.0f);

//distance of rays, will select the shortest in 3D
srange1 = sqrt((tstY - prevY)*(tstY - prevY) + (tstX - prevX)*(tstX - prevX) + (tstZ1 - prevZ)*(tstZ1 - prevZ));

This comment has been minimized.

Copy link
@Naios

Naios Nov 19, 2015

Contributor

Don't reinvent the wheel, use Vector3::GetLength or similar please.

@msoky

This comment has been minimized.

Copy link
Contributor

msoky commented Nov 28, 2015

any news?

@kvipka

This comment has been minimized.

Copy link
Author

kvipka commented Nov 28, 2015

Nope, no any trouble yet

@P-Kito

This comment has been minimized.

Copy link
Contributor

P-Kito commented Nov 29, 2015

Merge! ^^

@Keader

This comment has been minimized.

Copy link
Member

Keader commented Nov 29, 2015

I believe we are waiting the suggested modifications : #15796 (comment) #15796 (comment)

@FrancescoBorzi

This comment has been minimized.

Copy link
Member

FrancescoBorzi commented Nov 29, 2015

@kvipka please can you edit your code as suggested so your PR can be merged?

@FrancescoBorzi

This comment has been minimized.

Copy link
Member

FrancescoBorzi commented Dec 12, 2015

Tested on cc685ac with players and it seems to be working fine. @kvipka are you alive?

@msoky

This comment has been minimized.

Copy link
Contributor

msoky commented Dec 12, 2015

can we use it for 6.x?

@FrancescoBorzi

This comment has been minimized.

Copy link
Member

FrancescoBorzi commented Feb 11, 2016

yes maybe the best option is to merge this and then @Naios open a new PR in order to do all the refactoring

@P-Kito

This comment has been minimized.

Copy link
Contributor

P-Kito commented Feb 11, 2016

👍

@Treeston

This comment has been minimized.

Copy link
Member

Treeston commented Feb 11, 2016

@kvipka if you could finish your version yourself, that'd be preferred, of course ;)

Add available blink on Transport
* using Position

fix travis

Blink v4
- -[x] blink in abyss
- -[x] blink by path ( rechecking each step )
- -[x] where player can walk - blink will available there
- -[x] implemented check on dynamic objects (locked doors and etc)
- -[x] implemented total path of blink.

video : http://www.youtube.com/watch?v=wpymzrkwNTg
@kvipka

This comment has been minimized.

Copy link
Author

kvipka commented Feb 12, 2016

okay, did it in one commit

@Treeston

This comment has been minimized.

Copy link
Member

Treeston commented Feb 12, 2016

👍 awesome

@Tonghost

This comment has been minimized.

Copy link
Contributor

Tonghost commented Feb 13, 2016

@kvipka 𝓚𝓟𝓐𝓒𝓤𝓑𝓞, других слов не подобрать.

@Treeston

This comment has been minimized.

Copy link
Member

Treeston commented Feb 17, 2016

Been testing the latest version of this for the last days, and it works really really well. 👍 for great work.

@kvipka kvipka changed the title Blink v4 ... Feb 26, 2016
@kvipka kvipka closed this Feb 26, 2016
@P-Kito

This comment has been minimized.

Copy link
Contributor

P-Kito commented Feb 26, 2016

:(

@Aokromes Aokromes changed the title ... Blink v4 Feb 26, 2016
@Treeston Treeston mentioned this pull request Feb 26, 2016
@kvipka

This comment has been minimized.

Copy link
Author

kvipka commented Feb 27, 2016

stop it

@ghost

This comment has been minimized.

Copy link

ghost commented Feb 27, 2016

What is wrong, kvipka? Why did you close this and why don't you want it to continue to be used?

@w5860363

This comment has been minimized.

Copy link

w5860363 commented Feb 27, 2016

@kvipka Can cast sideways It is not possible at Blizzard.

@ghost

This comment has been minimized.

Copy link

ghost commented Feb 27, 2016

@w5860363 : I think we need to post in issue #16688 instead. I think this one will stay closed.

@kvipka

This comment has been minimized.

Copy link
Author

kvipka commented Feb 27, 2016

@w5860363 what?

@w5860363

This comment has been minimized.

Copy link

w5860363 commented Feb 27, 2016

@kvipka

This comment has been minimized.

Copy link
Author

kvipka commented Feb 27, 2016

@w5860363 what about you? orientation of blink depend only from orientation of character in game. It doesn't relate with trinity version of blink, or mine, or smthing else.

@Rushor

This comment has been minimized.

Copy link
Contributor

Rushor commented Feb 27, 2016

what's wrong @kvipka, why do you close/unassign all your good prs? :( makes me sad

@w5860363

This comment has been minimized.

Copy link

w5860363 commented Feb 27, 2016

@kvipka Indeed with the version independent,sorry

@FrancescoBorzi

This comment has been minimized.

Copy link
Member

FrancescoBorzi commented Feb 27, 2016

@kvipka keep up the good work and get this PR merged ;)

@kvipka

This comment has been minimized.

Copy link
Author

kvipka commented Feb 27, 2016

@ShinDarth just close it.
@w5860363 , idk, what about you, but on retail blink only at current orientation. In this version too. Without any deviations.

// upper or floor
destz2 = map->GetHeight(phasemask, destx, desty, prevZ, true, 25.0f);
//lower than floor
destz3 = map->GetHeight(phasemask, destx, desty, prevZ - maxtravelDistZ / 2, true, 25.0f);

This comment has been minimized.

Copy link
@r00ty-tc

r00ty-tc Feb 29, 2016

Contributor

For this, there's generally an issue with getHeight. Where it often returns a z that is below ground, if there's another vmap layer close to the current ground.

I've been working on a general solution to this. None so far have worked in a majority of cases but not use more CPU.. This falls into the uses more CPU bracket. For blink it's probably fine though. Unless someone wants to do a mage based DDoS.

This comment has been minimized.

Copy link
@kvipka

kvipka Feb 29, 2016

Author

@r00ty-tc do you know, how working getheight? It's vector from x,y,z down at FIRST collision. On one point x,y you can have more then 1,2,3,4 and etc points. It's need for correction z in difficult sutuation, for example tunnel on WSG.

About calls of this func... Have you anything looked in pathgenerator? Do you know, how many times this class calling updateAllowedPosition ? this func also calling getheight, and not only time....

p.s. anyway, i think, if you will inspect smthing and comment - the way for you in Naios repo. Not this.

This comment has been minimized.

Copy link
@r00ty-tc

r00ty-tc Feb 29, 2016

Contributor

Yep I know, and it can only do that due to the way textures are in game. I wrote a whole post this morning on TC forum about it

https://community.trinitycore.org/topic/12397-unit-z-position-and-creatureinvoluntary-player-movement/

This comment has been minimized.

Copy link
@kvipka

kvipka Feb 29, 2016

Author

yep, I have read. But you doesn't understand me.

"On one point x,y you can have more then 1,2,3,4 and etc points. It's need for correction z in difficult sutuation, for example tunnel on WSG."

if you will take getheight very high - you can take coord of ceiling of tunnel. if you will take coord lower then current z (previous step of blink) - it may be lower then available, and in this case getheight will sent new vector from max_height, it can give coord of upper ground, upper then tunnel. And when you will sent vector of collision from previous step at new point - you will take collision (when ceiling). And blink will stoped. Now understand?

This method doesn't related with your feature.

This comment has been minimized.

Copy link
@r00ty-tc

r00ty-tc Feb 29, 2016

Contributor

I understand. But in that situation, if you pick the x, y, z position based on current location and use mmap to derive a good Z. Then the LoS check should be in a downward gradient (following the tunnel) and therefore allow the blink. That is, you have start position, and end position. You can then get Height on both (or even ValidateZPosition) then LoS between the two points. As an alternative to the three beam method you're using.

The problem still starts with getHeight not always give an ideal result. As I allude to in the forum post. No solution that works in all situations takes the same or less CPU as currently though. It affects anything doing a movement of player or creature to a new position with no pre-determined Z position that we must derive.

This comment has been minimized.

Copy link
@kvipka

kvipka Feb 29, 2016

Author

Nope, you just can't understand, what about i talking (maybe my bad english =( )

Anyway, if this topic for conversation, look https://rustemu.org/threads/collision-check-in-target-movement-generator.29/ . This bug and fix also doesn't related with your catch ( https://community.trinitycore.org/topic/12397-unit-z-position-and-creatureinvoluntary-player-movement/ )

This comment has been minimized.

Copy link
@r00ty-tc

r00ty-tc Feb 29, 2016

Contributor

Well that thread also doesn't help me understand. The NPCs falling down are exactly what I was aiming to fix with getHeight changes. I cannot see the github links because you removed all of the commits.

The commit I note in the thread is the "fast" correction for a very old bug. And it solves let's say 50% of the times it goes wrong. The rest are caused by the vmap lookup (always looking down from 2.0f above). This is flawed when the terrain between two steps changes by more than 2.0f. The solution I'm advocating (and have tested to a certain extent) uses mmap and a search above and below to find nearest vmap/map floor to the mmap floor. Which resolves most of these. I'm not exactly sure of the location you are using in that demo video.

Like I say, unless there's some other point you're trying to make and I'm not seeing it in that thread, the getHeight solution (including the other points not included in a commit yet) should solve them.

This comment has been minimized.

Copy link
@kvipka

kvipka Feb 29, 2016

Author

Borean Tundra, near the Aliance castle, upper then mines.

This comment has been minimized.

Copy link
@r00ty-tc

r00ty-tc Feb 29, 2016

Contributor

So https://www.youtube.com/watch?v=YG5n_orJF-c with only the first basic correction applied. You see it falls once, but the subsequent changes I plan will solve that too. In fact, I think that serverside that didn't actually happen and that it's a result of mmaps producing pathing that the client doesn't accept and correctly shows falling for, which is corrected at the next path point. e.g. I am saying the path generated should have gone around that hole.

That is another story entirely though!

This comment has been minimized.

Copy link
@kvipka

kvipka Feb 29, 2016

Author

yep, for movement and declare first try point - rly good. My solution was related with check on LoS, but with your feature - now it's only safe check, and now movegen has a little count of call func. Rly good catch. But anyway - for blink it doesn't matter =)

@ChazyTheBest

This comment has been minimized.

Copy link
Contributor

ChazyTheBest commented Mar 7, 2016

If it helps, I remember you could pass trough an instance portal without triggering it, either way in or out. I can't remember the expansion/version.

@msoky

This comment has been minimized.

Copy link
Contributor

msoky commented Jun 5, 2016

if is that better and less buged than blink which is actually used, then maybe someone can push new blink system and fix minor bugs later..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.