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

Core/VMaps: Fix no collision triangles #20024

Merged
merged 1 commit into from Nov 22, 2017
Merged

Conversation

Golrag
Copy link
Contributor

@Golrag Golrag commented Jul 14, 2017

Changes proposed:

  • Fix collision check when generating vmaps

Target branch(es): 3.3.5/master

  • 3.3.5
  • master

Issues addressed: Closes #19865

Tests performed: (Does it build, tested in-game, etc.)

  • Builds
  • Tested in game

Notes:

  • Reextracting vmaps

@ccrs
Copy link
Member

ccrs commented Jul 14, 2017

wrong

just remove c357939#diff-97edc20817c42622adaefabfd511126fR514

leave the rest

@Golrag
Copy link
Contributor Author

Golrag commented Jul 14, 2017

Reverted my changes, applied what you suggested but then SotA bugged again

@ccrs
Copy link
Member

ccrs commented Jul 14, 2017

you cant have both at the same time, sota bug is caused by paths not being calculated under a dynamic environment but a static one. That line ignores dynamic wmos so the core can calculate those dynamic paths without restrictions.

so yeah, cant have it till the core functionality is developed

@Warpten
Copy link
Member

Warpten commented Jul 15, 2017

you cant have both at the same time, sota bug is caused by paths not being calculated under a dynamic environment but a static one. That line ignores dynamic wmos so the core can calculate those dynamic paths without restrictions.

so yeah, cant have it till the core functionality is developed

this is true iff the wmo giving mmap support on LK is destructible, and im not sure it is. i dont see why blizzard would make it destructible anyway because it doesnt provide clientside collision asn i very much pointed out by copy-pasting clientside checks into the extractor.

@Golrag
Copy link
Contributor Author

Golrag commented Jul 16, 2017

Not sure if this should be intended:
http://i.imgur.com/lp9pi1i.jpg
Also, the rock does not provide LoS (and it's not a gameobject)

@Warpten
Copy link
Member

Warpten commented Jul 16, 2017

I know for a fact the trees in elwynn forest aren't supposed to give LoS. Last I checked they did on TC

@Killyana
Copy link
Member

m2 doesn't provide LoS #19185

@Golrag
Copy link
Contributor Author

Golrag commented Jul 17, 2017

Trees in Elwynn forest still do with this PR. http://i.imgur.com/REj0gah.jpg
I also noticed that without this PR, the if-test was never true (on wotlk)
List of affected wmos (7K+ all of them ?): https://gist.github.com/Golrag/06d58e3d2432a5e53acd67e28aa99401

@Aokromes
Copy link
Member

@Golrag jump into irc :)

@Warpten
Copy link
Member

Warpten commented Jul 17, 2017

Here is the trick, there are M2s part of wmos, and I'm not sure the extractor is smart enough to pull them out of the WMO (even if it is just a simple matrix operation). Not that it affects elwynn forest trees, but that is something worth considering. Terrain stuff isn't all black and white.

@akrom23
Copy link
Contributor

akrom23 commented Jul 23, 2017

So, it's a valid fix or not? This bug causes many problems with destructible gos.

@Golrag
Copy link
Contributor Author

Golrag commented Jul 24, 2017

Don't think M2s are affected by this, with or without this PR trees in elwynn forest are extracted, the spell system just ignores M2 when LoS checking.
I just implemented the previous check back with updated enum values which matches the comment // Skip no collision triangles and https://wowdev.wiki/WMO/v17#MOPY_chunk

bool isRenderFace() { return F_RENDER && !F_DETAIL; }
bool isCollidable() { return F_COLLISION || isRenderFace(); }

@Aokromes
Copy link
Member

@akrom23 what problems it causes?

@Golrag
Copy link
Contributor Author

Golrag commented Jul 29, 2017

I think he is talking about #19865

@akrom23
Copy link
Contributor

akrom23 commented Jul 30, 2017

Yes.

@Keader
Copy link
Member

Keader commented Aug 24, 2017

sould fix #19976 too

@Golrag
Copy link
Contributor Author

Golrag commented Aug 24, 2017

No, as some point combinations (x,y) with ToCCommonLoc[1].GetPositionZ() are going slightly under the gameobject (it's not a flat gameobject) which results in looking at the floor under the one we actually want. I proposed a fix where we just add 5.f but it doesn't feel like a proper fix,

@Warpten
Copy link
Member

Warpten commented Aug 27, 2017

Do I still need to look into this? Kind of forgot about it and moved away from wow for a couple months

I kind of forgot the details but this commit would only fix a bug on toc if the floor was actually a destructible WMO. Same for lk. I don't see why blizzard would bother for both of these since they are pretty much flat geometries. On both of those you fall down anyway when it destroys. I'd actually be interested to see what happens if you trigger a phase transition end on lk with trashs still up on retail then jump down to your death. My money is on the trash not trying to follow you down.

Also maybe consider a) not load WMO geometry if a gameobject with corresponding model is spawned there, or b) straight up not extract destructibles on bg maps.

@Golrag +5 is fine for all I care since UpdatAllowedPositionZ will clamp to the ground

@ccrs
Copy link
Member

ccrs commented Aug 27, 2017

#20024 (comment)

The problem is pathgenerator not the extractor, so either revert the logic so the data takes into account dynamic situations or fix pathgenerator.

@Warpten
Copy link
Member

Warpten commented Aug 28, 2017

Again, no. Client map files (.adts) have models for the doors spawned in, it just so happens that everything gets flagged as no-render. This is very easily seen in terrain editors such as noggit (you're welcome, @bloerwald <3)

This is your issue here. It's not caused by dynamic los. It's caused by duplicate data where the static tree has references to the wmos that the dynamic tree also has. When the door is destroyed the dynamic tree is correctly registering it, but the static one gives no flying fucks and keeps thinking the door is up, causing a raycast hit and thus line of sight issues.

The only sensible fix is to remove destructible wmos from the static tree. If you need pathfinding on destructible floors, then implement that. That's something I have no clue how to do, I barely understand recast. I think it supports dynamic geometry, but I'm not sure how to use it and if it'll even be good enough for us. And again, I would not be surprised if toc and lk floor was handled as non-destructible by blizzard to keep it easy on themselves.

[Edit] I remember now, every time you want to change the geometry you have to regenerate a tile. Doing this at runtime is unacceptable, thus no using that.

@Warpten
Copy link
Member

Warpten commented Aug 28, 2017

I will finally take the time to check this all tomorrow.

@Golrag
Copy link
Contributor Author

Golrag commented Oct 21, 2017

This location https://puu.sh/y3yEZ/2850816ad5.png
should not provide height data (no path, and npc's fall through it on retail).
Yet with my PR here it does :(
Will test soon if it ignore height data without this PR (might take some time)
Edit:
Same thing without this PR

@Warpten
Copy link
Member

Warpten commented Oct 27, 2017

its returning ground 60 feet below, its fine

@Golrag
Copy link
Contributor Author

Golrag commented Oct 27, 2017

Okay, tried the position I used in my example here on retail: #19865 (comment)
http://i.imgur.com/xvGqDnk.jpg

Got on the spike with a demon hunter, tried to meta on it while I was standing on the spike, no path so I am assuming this is working as intended.

Just trying to find things that might be broken due to this PR. Not easy to do tho. I have to check if it was present without this PR and if there is a change, I have to check retail. I think that's the safest way to check if the PR didn't change things in an unintended way, but time consuming & nearly impossible alone. So I started looking for interesting spots on retail and checked if those were working as intended with/without this PR :p (only really interesting point was the door in black rock (spire?) where you can walk on it and I found that spot in darnassus).

I wish I could be a better help with this issue tho, learning all that stuff and find a way to get (& visualise ?) all the data from wow but I'm afraid I don't have the knowledge & tools (yet).

@Warpten is there something holding you back from accepting this PR ? Would like to know what it is if you can share it. If it's just some testing I can try to make some free time for this and maybe put up a nice list of differences between: current rev, this PR and retail.

@ghost ghost added the Sub-Maps/MMaps/Vmaps label Oct 27, 2017
@Faq
Copy link
Contributor

Faq commented Nov 18, 2017

@Warpten any comment on this?

@Warpten
Copy link
Member

Warpten commented Nov 21, 2017

Yellow is regular terrain. Red is gameobjects. Green is WMO/M2s tied to ADT (static spawns). Blue will be water, if I get around to that.

SotA

ICC - Arthas Platform

ToTC:

Gameobjects rendered

Gameobjects not rendered

Being AFK during acidmaw+dreadscale didn't show any bug, AFAIK. After burrows, everything went OK.
Icehowl was not tested. Nothing could possibly have changed to impact his fight.
Jesus christ that gnome takes his sweet ass time.

This is how it looks when the gameobject falls down

Yes, there's ADT floor here. Yes, it will provide pathfinding if the gameobject can't. it is however slightly below the normal level of the gameobject, so one can wonder what blizzard were thinking here.

TL;DR: This is good to go in my eyes

@Warpten
Copy link
Member

Warpten commented Nov 22, 2017

Regarding ADT floor: appears to be a possible bug in my code, as every single cave entrance is affected. Unrelated to issue.

@Warpten Warpten merged commit 9614f9d into TrinityCore:3.3.5 Nov 22, 2017
@Golrag Golrag deleted the fix_collision branch November 22, 2017 16:14
livingsacrifice pushed a commit to livingsacrifice/TrinityCore that referenced this pull request Mar 16, 2018
Core/VMaps: Fix no collision triangles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants