Page MenuHomeWildfire Games
Differential D1270

SoundGroup cleanup.
ClosedPublic
Actions

Authored by Stan on Jan 28 2018, 11:03 PM.

Details

Reviewers
vladislavbelov
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22009: SoundGroup cleanup.
Summary

Those files are quite old, and haven't been modified since, hence there some useless stuff in there.

This patch aims to clean the code up, remove redundancy, enforce coding conventions.

So far, removed useless include, renamed members consistently and ordered them,
Remove hardcoded pi constant and use M_PI as done elsewhere.
Replaced NULL by nullptr, Should I just go for !variable ?
Should I use iterators where possible ?

Should I make a getSchema static method as it's done for other components ?

Test Plan

Test that nothing changed.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Jan 29 2018, 12:07 AM
source/soundmanager/scripting/SoundGroup.cpp
104 ↗(On Diff #5555)

Wasn't this the file with PI = 3 but then it turned out that it was unused, then it turned out that it should be used and we just don't do anything about it? lepers patch somewhere?

178 ↗(On Diff #5555)

unneeded parentheses

200 ↗(On Diff #5555)

is that conversion needed?

208 ↗(On Diff #5555)

Isn't that already size_t?

220 ↗(On Diff #5555)

Would have suggested ++i; if I hadn't suggeted a range-based loop, same below

Stan marked 4 inline comments as done.Jan 29 2018, 12:24 AM
Stan added inline comments.
source/soundmanager/scripting/SoundGroup.cpp
104 ↗(On Diff #5555)

Indeed that was that file. Patch was by me btw.

200 ↗(On Diff #5555)

Apparently so g_SoundManager is a ISoundManager and doesn't have a PlayGroupItem public member.

208 ↗(On Diff #5555)

Nice catch.

Stan updated this revision to Diff 5556.Jan 29 2018, 12:25 AM
Stan marked 2 inline comments as done.

Fix above comment. Thanks for the feedbacks guys :D

Vulcan added a comment.Jan 29 2018, 1:13 AM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Jan 29 2018, 1:15 AM
Executing section Default...
Executing section Source...
Executing section JS...
Harbormaster completed remote builds in B4719: Diff 5556.Jan 29 2018, 1:15 AM
Stan edited reviewers, added: Restricted Owners Package; removed: Itms.Feb 18 2018, 5:10 PM
Stan updated this revision to Diff 5926.Feb 25 2018, 4:28 AM

Some more cleanup

Stan added a reviewer: vladislavbelov.Feb 25 2018, 4:38 AM
Vulcan added a comment.Feb 25 2018, 4:33 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/95/display/redirect

Harbormaster completed remote builds in B5206: Diff 5926.Feb 25 2018, 4:33 PM
vladislavbelov requested changes to this revision.Mar 1 2018, 9:30 PM
vladislavbelov added inline comments.
source/soundmanager/scripting/SoundGroup.cpp
83 ↗(On Diff #5926)

It's better to move x, y to the place exactly before a usage. Because currently it looks strange.

84 ↗(On Diff #5926)

The same here.

134 ↗(On Diff #5926)

isOnScreen?

242 ↗(On Diff #5926)

Why this was removed? It shouldn't be, because it's supported compilation flag. Or remove everywhere, but it's not recommended.

265 ↗(On Diff #5926)

The same here.

104 ↗(On Diff #5555)

Do we always have M_PI, it isn't in the standard.

source/soundmanager/scripting/SoundGroup.h
57 ↗(On Diff #5926)

Why it was removed?

99 ↗(On Diff #5926)

It should be:

#endif // INCLUDED_SOUNDGROUP_H
This revision now requires changes to proceed.Mar 1 2018, 9:31 PM
Stan added inline comments.Mar 1 2018, 9:35 PM
source/soundmanager/scripting/SoundGroup.cpp
134 ↗(On Diff #5926)

It's set below ?

242 ↗(On Diff #5926)

AFAIK It's set everywhere to 1

104 ↗(On Diff #5555)

We use it everywhere else

source/soundmanager/scripting/SoundGroup.h
57 ↗(On Diff #5926)

Not used ?

Stan updated this revision to Diff 6335.Apr 7 2018, 12:22 PM

Fix the above comments and one I missed from elexis.

Vulcan added a comment.Apr 7 2018, 12:26 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/354/display/redirect

Harbormaster completed remote builds in B5782: Diff 6335.Apr 7 2018, 12:26 PM
wraitii requested changes to this revision.May 15 2018, 8:57 PM
wraitii added a subscriber: wraitii.

Other remarks.

I'm not sure what config2.h is, but it does look like these CONFIG2_AUDIO defines could be removed altogether.

source/soundmanager/scripting/SoundGroup.cpp
40 ↗(On Diff #6335)

Don't think that's really something we do in general, and it's not very useful over such a small distance.

89 ↗(On Diff #6335)

This isn't how we cast things, prefer (float)

Also this cast isn't needed at all, same for above, so long as you divide by a float. So switch to M_PI / 3.f

111 ↗(On Diff #6335)

Drop redundant parentheses.

84 ↗(On Diff #5926)

I think the doubt on M_PI is on visual studio. If it's been added now it ought to be safe to use.

source/soundmanager/scripting/SoundGroup.h
22 ↗(On Diff #6335)

Re-add the description above the class name, as a regular old comment.

106 ↗(On Diff #6335)

Switch this to uint8_t

This revision now requires changes to proceed.May 15 2018, 8:57 PM
Stan planned changes to this revision.May 15 2018, 9:24 PM

I'll remove CONFIG2_AUDIO in another diff if it is indeed not being used by premake or anything else.

Stan updated this revision to Diff 6555.May 15 2018, 9:25 PM

Fix the above comments.

wraitii accepted this revision.May 15 2018, 9:29 PM

you've missed a static_cast on L87
and some more redundant parens on L111 on the right

Otherwise this works and is better so go ahead.

Vulcan added a comment.May 15 2018, 9:31 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/497/display/redirect

Harbormaster completed remote builds in B6069: Diff 6555.May 15 2018, 9:31 PM
Stan updated this revision to Diff 6556.May 15 2018, 9:37 PM

Remove a cast

Vulcan added a comment.May 15 2018, 9:40 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/498/display/redirect

Harbormaster completed remote builds in B6070: Diff 6556.May 15 2018, 9:40 PM
Stan updated this revision to Diff 6557.May 15 2018, 9:47 PM

simplify math duh.

Vulcan added a comment.May 15 2018, 9:50 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/499/display/redirect

Harbormaster completed remote builds in B6071: Diff 6557.May 15 2018, 9:50 PM
Stan mentioned this in D1502: Remove some unused functions.May 19 2018, 4:30 PM
Stan mentioned this in D1584: Seed random sounds.Jun 21 2018, 2:58 PM
vladislavbelov added inline comments.Jan 2 2019, 8:59 PM
source/soundmanager/scripting/SoundGroup.cpp
1 ↗(On Diff #6557)

Year :)

38 ↗(On Diff #6557)

Actually static_cast isn't needed here, because you already divided the rand by float.

50 ↗(On Diff #6557)

It seems it was never used: https://trac.wildfiregames.com/changeset/12428

89 ↗(On Diff #6557)

I don't believe for M_PI until it presents in the standard. Wouldn't it better to use our own constant in lib? It'd allow to use #ifndef M_PI to guarantee the existing value.

133 ↗(On Diff #6557)

Missed renaming. Did you try to compile with disabled CONFIG2_AUDIO?

147 ↗(On Diff #6557)

if (m_SoundGroups.empty()).

204 ↗(On Diff #6557)

if (m_Filenames.empty()).

238 ↗(On Diff #6557)

& isn't needed here, just for (CSoundData* soundGroup : m_SoundGroups). Because you don't need to change the pointer, you need to change the object.

source/soundmanager/scripting/SoundGroup.h
1 ↗(On Diff #5554)

Year again :)

Stan updated this revision to Diff 7200.Jan 2 2019, 9:34 PM
Stan marked 2 inline comments as done.

Bump year, fix above comments.

Vulcan added a comment.Jan 2 2019, 9:34 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/879/

Harbormaster failed remote builds in B6660: Diff 7200!Jan 2 2019, 9:34 PM
Stan marked 25 inline comments as done.Jan 2 2019, 9:36 PM
Stan added inline comments.
source/soundmanager/scripting/SoundGroup.cpp
50 ↗(On Diff #6557)

Indeed

133 ↗(On Diff #6557)

Yep it works now.

vladislavbelov added inline comments.Jan 2 2019, 9:43 PM
source/soundmanager/scripting/SoundGroup.cpp
50 ↗(On Diff #6557)

I found usage: https://trac.wildfiregames.com/changeset/7609#file25

vladislavbelov accepted this revision.Jan 2 2019, 9:44 PM
This revision is now accepted and ready to land.Jan 2 2019, 9:44 PM
vladislavbelov added inline comments.Jan 2 2019, 10:22 PM
source/soundmanager/scripting/SoundGroup.cpp
42 ↗(On Diff #7200)

It seems it have to be left.

Stan updated this revision to Diff 7201.Jan 2 2019, 10:37 PM
Stan marked an inline comment as done.

Thanks @Vulcan

Vulcan added a comment.Jan 2 2019, 10:43 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/880/

Harbormaster failed remote builds in B6662: Diff 7201!Jan 2 2019, 10:43 PM
Vulcan added a comment.Jan 2 2019, 10:46 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/881/

Vulcan added a comment.Jan 2 2019, 11:25 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/883/

Harbormaster completed remote builds in B6662: Diff 7201.Jan 2 2019, 11:25 PM
Closed by commit rP22009: SoundGroup cleanup. (authored by Stan). · Explain WhyJan 2 2019, 11:32 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
ps/
trunk/
source/
soundmanager/
scripting/
90 lines
242 lines

Diff 7203

View Options

ps/trunk/source/soundmanager/scripting/SoundGroup.h

Loading...
View Options

ps/trunk/source/soundmanager/scripting/SoundGroup.cpp

Loading...
Wildfire Games · Phabricator