Fix units not being able to play sounds when walking and running
AuditedrP21359
Actions

Description

Fix units not being able to play sounds when walking and running
This currently disabled by variants, and will be reenabled when sounds have been chosen.
Reviewed by: Vladislav,
Comments by: Itms, elexis
Differential Revision: https://code.wildfiregames.com/1257

Details

Auditors

temple
mimo
elexis

Committed

Stan	Feb 24 2018, 7:13 PM

Reviewer

Itms

Differential Revision

D1257: Fix Soundgroups not playing sounds

Parents

rP21358: petra fixes/improvments of rP21346

Branches

Unknown

Tags

Unknown

Build Status

Buildable 5190
Build 8870: Trigger Windows Autobuild
Build 8869: Post-Commit Build	Jenkins

Event Timeline

Stan committed rP21359: Fix units not being able to play sounds when walking and running.Feb 24 2018, 7:13 PM

Imarok added an edge: D1257: Fix Soundgroups not playing sounds.Feb 24 2018, 9:03 PM

Stan mentioned this in rP21361: Fix broken tests,.Feb 24 2018, 9:38 PM

Harbormaster failed to build B5190: rP21359: Fix units not being able to play sounds when walking and running!Feb 25 2018, 3:24 AM

autostart-nonvisual are broken, and i guess because of that patch

For example, using

gdb --args pyrogenesis -autostart=random/mainland -autostart-players=4 -autostart-ai=1:petra -autostart-civ=1:sele -autostart-ai=2:petra -autostart-civ=2:ptol -autostart-ai=3:petra -autostart-civ=3:mace -autostart-ai=4:petra -autostart-civ=4:ptol -autostart-nonvisual -autostart-victory=conquest

we have

Thread 1 "pyrogenesis" received signal SIGSEGV, Segmentation fault.
0x00005555556e8ac3 in CmpPtr<ICmpSound>::CmpPtr (ent=<optimized out>, context=..., this=<synthetic pointer>)
    at ../../../source/simulation2/system/CmpPtr.h:63
63                      m = static_cast<T*>(QueryInterface(context, ent, T::GetInterfaceId()));
(gdb) bt
#0  0x00005555556e8ac3 in CmpPtr<ICmpSound>::CmpPtr (ent=<optimized out>, context=..., this=<synthetic pointer>)
    at ../../../source/simulation2/system/CmpPtr.h:63
#1  CCmpVisualActor::SelectAnimation (this=0x555556d6f5a0, name=..., once=<optimized out>, speed=...)
    at ../../../source/simulation2/components/CCmpVisualActor.cpp:451
#2  0x00005555556e762d in CCmpVisualActor::Init (this=0x555556d6f5a0, paramNode=...)
    at ../../../source/simulation2/components/CCmpVisualActor.cpp:210
#3  0x0000555555635923 in CComponentManager::AddComponent (this=this@entry=0x5555562964d8, ent=..., cid=<optimized out>, 
    paramNode=...) at ../../../source/simulation2/system/ComponentManager.cpp:711
#4  0x0000555555635d63 in CComponentManager::AddEntity (this=0x5555562964d8, templateName=L"structures/ptol_civil_centre", ent=150)
    at ../../../source/simulation2/system/ComponentManager.cpp:885
#5  0x00005555555fd672 in CSimulation2::AddEntity (this=<optimized out>, templateName=L"structures/ptol_civil_centre", 
    preferredId=<optimized out>) at ../../../source/simulation2/Simulation2.cpp:679
#6  0x00005555558b3712 in CMapReader::ParseEntities (this=0x55555621f000) at ../../../source/graphics/MapReader.cpp:1375
#7  0x00005555558a8824 in MemFunThunk<CMapReader> (param=0x5555562cb2e0) at ../../../source/ps/LoaderThunks.h:60
#8  0x00005555557f5c82 in LDR_ProgressiveLoad (time_budget=time_budget@entry=100, 
    description=description@entry=0x7fffffffca20 L"CMapReader::GenerateMap", max_chars=max_chars@entry=100, 
    progress_percent=progress_percent@entry=0x7fffffffca1c) at ../../../source/ps/Loader.cpp:228
#9  0x00005555557f6085 in LDR_NonprogressiveLoad () at ../../../source/ps/Loader.cpp:318
#10 0x0000555555811fba in Autostart (args=...) at ../../../source/ps/GameSetup/GameSetup.cpp:1536
#11 0x00005555555b124d in RunGameOrAtlas (argc=<optimized out>, argv=<optimized out>) at ../../../source/main.cpp:581
#12 0x00005555555a1c97 in main (argc=13, argv=0x7fffffffd8c8) at ../../../source/main.cpp:632

This commit now has outstanding concerns.Feb 25 2018, 10:34 AM

https://code.wildfiregames.com/1257

(Missing D)

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
4260	Good, wanted to comment on that already. But on the other hand it would have been nice to split the logic change from the cleanup.
/ps/trunk/source/tools/atlas/GameInterface/ActorViewer.cpp
434	from rP7609

Stan mentioned this in rP21375: Check for m_Unit before using it as it is done below..Feb 25 2018, 3:58 PM

Thanks for fixing it

All concerns with this commit have now been addressed.Feb 25 2018, 8:33 PM

temple raised a concern with this commit.May 24 2018, 1:55 AM

temple added a subscriber: temple.

temple added inline comments.

/ps/trunk/source/simulation2/components/CCmpVisualActor.cpp
446	`m_SoundGroup = ""` now for non-visual replays since they don't have `m_Unit` (see rP21375), which causes a problem since `m_SoundGroup` is serialized. So it's easy to get hash mismatches with the full hash (apparently it's not used in the quick hash), which makes it harder to diagnose other oos issues.

This commit now has outstanding concerns.May 24 2018, 1:55 AM

Stan added inline comments.May 24 2018, 9:21 AM

/ps/trunk/source/simulation2/components/CCmpVisualActor.cpp
446	So I should set it to name by default ?

wraitii added a subscriber: wraitii.May 24 2018, 9:42 AM

wraitii added inline comments.

/ps/trunk/source/simulation2/components/CCmpVisualActor.cpp
451	Why are we using m_Unit->GetID() here?

Stan added inline comments.May 24 2018, 9:45 AM

/ps/trunk/source/simulation2/components/CCmpVisualActor.cpp
451	Not sure I believe it was initialized that way somewhere else.

See D1519 for a potential fix.

wraitii added inline comments.May 24 2018, 10:12 AM

/ps/trunk/source/simulation2/components/CCmpVisualActor.cpp
451	Because if we used GetEntityHandle() I don't think we need to rely on m_Unit at all and then you can fix D1519 this way instead.

Stan added inline comments.May 24 2018, 10:15 AM

/ps/trunk/source/simulation2/components/CCmpVisualActor.cpp
451	Considering the below check (m_Unit && m_Unit->GetAnimation()) I think that would be saner... >< So: CmpPtr<ICmpSound> cmpSound(GetSimContext(), GetEntityHandle()); Correct ?