0000220: List Possible Songs

(0000653)
DevilHunter (reporter)
2010-12-04 18:12

Sounds good, and yet Zdaemon did something almost like this in a way...

"Extended the changemus command to show the name of the current song when no argument is specified."

I would say this would go with the request, but then, Zdaemon would say we are copying them.

(0000654)
Edward-san (developer)
2010-12-04 22:04

DH: No need to bother them, see here:
'http://mancubus.net/svn/zdoom?op=revision&rev=2782 [^]'
and here:
'http://mancubus.net/svn/zdoom?op=revision&rev=2902 [^]'

but the request is different, he requests a listings of all the music available... it should be "Ask Zdoom"'d IMHO

(0008881)
Watermelon (developer)
2014-06-07 21:50
edited on: 2014-06-07 21:50

I've wanted this for a long time, seeing this ticket inspired me to get this done (plus it's a simple addition too!):

'https://bitbucket.org/ChrisKOmg/zandronum/commits/93222c39c714a537a0648aef1b28331047b18a33 [^]'

(0008886)
Dusk (developer)
2014-06-07 22:44

What if a WAD contains multiple maps with the same music track? Wouldn't these wind up being printed twice?

(0008892)
Watermelon (developer)
2014-06-08 00:24

Yes, good point. Hashmap time.

(0008927)
Watermelon (developer)
2014-06-08 17:43

Fixed:
'https://bitbucket.org/ChrisKOmg/zandronum/commits/fe9b799a8d40e71feae5fa079a20e351136e8d61 [^]'

One last issue:
When running it in any wad it comes out fine, but in DOOM2.WAD it will list the musics with a $music_ prefix (ex: $music_runnin). Is that okay to be left in there, or should there be some replacement of "$music" with "d"?

(0008946)
Watermelon (developer)
2014-06-09 01:50

Note to self:
$music is a gstring / lang lump

(0008947)
Edward-san (developer)
2014-06-09 02:35
edited on: 2014-06-09 02:36

You should find some inspiration in function S_ChangeMusic in s_sounds.cpp. There's a specific handle of '$' char in the music name string.

(0009638)
Watermelon (developer)
2014-06-22 05:36

'https://bitbucket.org/Torr_Samaho/zandronum-stable/pull-request/53/added-a-new-ccmd-listmusic-which-dumps-the/diff [^]'

Works well. I did it the same way that Randy coded it for S_ChangeMusic in terms of determining what to do with GStrings.

(0009655)
Torr Samaho (administrator)
2014-06-22 13:56

The implementation itself looks good, there are just two minor style issues:

- Don't remove the new lines in the includes. We inherited those from (G)ZDoom and should keep them. If you need to add includes, just add them at the end of the Zandronum specific includes. In this case, this is after "#include "network/nettraffic.h". The next include "g_hub.h" still needs to be separated by a new line.
- Code consistency: You use both "(*it)." and "it->". While they should be equivalent, IMO it would be nicer if you decide for one and only use that in such a short code snippet.

(0009666)
Watermelon (developer)
2014-06-22 15:44
edited on: 2014-06-22 15:45

All fixed.
Now, there was one that used (*it)[0] instead of it->operator[](0), but I assume the former is much more clear and we'd be fine with this exception to the rule? The rest of course are all standardized to ->
Also added zan history update since it appears to be finalized.

'https://bitbucket.org/Torr_Samaho/zandronum-stable/pull-request/53/added-a-new-ccmd-listmusic-which-dumps-the/diff [^]'

(0009670)
Torr Samaho (administrator)
2014-06-22 16:24

Quote from Watermelon
All fixed.

Almost ;). With my comment "In this case, this is after "#include "network/nettraffic.h". The next include "g_hub.h" still needs to be separated by a new line." I tried to say that the include has to be in between "network/nettraffic.h" and "g_hub.h". This way it's with the other Zandronum includes. With your changes we would have "Zandronum includes, (G)ZDoom includes, Zandronum includes".

Quote from Watermelon
Now, there was one that used (*it)[0] instead of it->operator[](0)

Sure, that's fine. I mainly wondered about the difference between the two "GetChars", that's the only reason why I noticed the inconsistency.

(0009677)
Watermelon (developer)
2014-06-22 18:07

Just so I do it right this time (gotta make this one be the good commit!), is this what you mean

#include "network/nettraffic.h"
// [CK] For CCMD listmusic
#include <set>

#include "g_hub.h"

#include "gl/gl_functions.h"

(0009678)
Torr Samaho (administrator)
2014-06-22 18:10

Exactly. Marking this include is not necessary, since the whole block is already marked as Zandronum includes a few lines above, but you can keep the comment to stress that the STL was introduced here for listmusic.

(0009682)
Watermelon (developer)
2014-06-22 21:36

Nice and submitted

(0009714)
Torr Samaho (administrator)
2014-06-23 19:01

Compressed and rebased.

(0009828)
Arco (updater)
2014-07-03 19:33

Works in V1.3 140703-1806.

This issue is already marked as resolved. If you feel that is not the case, please reopen it and explain why.
Supporters:	No one explicitly supports this issue yet.
Opponents:	No one explicitly opposes this issue yet.

Issue History
Date Modified	Username	Field	Change
2010-12-02 21:13	Anonymous	New Issue
2010-12-04 18:12	DevilHunter	Note Added: 0000653
2010-12-04 22:04	Edward-san	Note Added: 0000654
2014-06-07 21:50	Watermelon	Note Added: 0008881
2014-06-07 21:50	Watermelon	Assigned To	=> Watermelon
2014-06-07 21:50	Watermelon	Status	new => needs review
2014-06-07 21:50	Watermelon	Note Edited: 0008881	View Revisions
2014-06-07 22:44	Dusk	Note Added: 0008886
2014-06-08 00:24	Watermelon	Note Added: 0008892
2014-06-08 00:29	Watermelon	Status	needs review => assigned
2014-06-08 17:43	Watermelon	Note Added: 0008927
2014-06-08 17:43	Watermelon	Status	assigned => needs review
2014-06-09 01:50	Watermelon	Note Added: 0008946
2014-06-09 02:35	Edward-san	Note Added: 0008947
2014-06-09 02:36	Edward-san	Note Edited: 0008947	View Revisions
2014-06-15 22:16	Watermelon	Status	needs review => assigned
2014-06-22 05:36	Watermelon	Note Added: 0009638
2014-06-22 05:36	Watermelon	Status	assigned => needs review
2014-06-22 13:56	Torr Samaho	Note Added: 0009655
2014-06-22 13:57	Torr Samaho	Status	needs review => feedback
2014-06-22 14:10	Dusk	Description Updated	View Revisions
2014-06-22 15:44	Watermelon	Note Added: 0009666
2014-06-22 15:45	Watermelon	Note Edited: 0009666	View Revisions
2014-06-22 15:47	Watermelon	Status	feedback => needs review
2014-06-22 16:24	Torr Samaho	Note Added: 0009670
2014-06-22 16:24	Torr Samaho	Status	needs review => feedback
2014-06-22 18:07	Watermelon	Note Added: 0009677
2014-06-22 18:07	Watermelon	Status	feedback => needs review
2014-06-22 18:10	Torr Samaho	Note Added: 0009678
2014-06-22 18:11	Torr Samaho	Status	needs review => feedback
2014-06-22 20:36	Watermelon	Status	feedback => needs review
2014-06-22 21:36	Watermelon	Note Added: 0009682
2014-06-23 19:01	Torr Samaho	Note Added: 0009714
2014-06-23 19:01	Torr Samaho	Status	needs review => needs testing
2014-06-23 19:01	Torr Samaho	Target Version	=> 1.3
2014-07-03 19:33	Arco	Note Added: 0009828
2014-07-03 19:34	Arco	Status	needs testing => resolved
2014-07-03 19:34	Arco	Resolution	open => fixed
2018-09-30 22:38	Blzut3	Status	resolved => closed

Notes
(0000653) DevilHunter (reporter) 2010-12-04 18:12	Sounds good, and yet Zdaemon did something almost like this in a way... "Extended the changemus command to show the name of the current song when no argument is specified." I would say this would go with the request, but then, Zdaemon would say we are copying them.

(0000654) Edward-san (developer) 2010-12-04 22:04	DH: No need to bother them, see here: 'http://mancubus.net/svn/zdoom?op=revision&rev=2782 [^]' and here: 'http://mancubus.net/svn/zdoom?op=revision&rev=2902 [^]' but the request is different, he requests a listings of all the music available... it should be "Ask Zdoom"'d IMHO

(0008881) Watermelon (developer) 2014-06-07 21:50 edited on: 2014-06-07 21:50	I've wanted this for a long time, seeing this ticket inspired me to get this done (plus it's a simple addition too!): 'https://bitbucket.org/ChrisKOmg/zandronum/commits/93222c39c714a537a0648aef1b28331047b18a33 [^]'

(0008886) Dusk (developer) 2014-06-07 22:44	What if a WAD contains multiple maps with the same music track? Wouldn't these wind up being printed twice?

(0008892) Watermelon (developer) 2014-06-08 00:24	Yes, good point. Hashmap time.

(0008927) Watermelon (developer) 2014-06-08 17:43	Fixed: 'https://bitbucket.org/ChrisKOmg/zandronum/commits/fe9b799a8d40e71feae5fa079a20e351136e8d61 [^]' One last issue: When running it in any wad it comes out fine, but in DOOM2.WAD it will list the musics with a $music_ prefix (ex: $music_runnin). Is that okay to be left in there, or should there be some replacement of "$music" with "d"?

(0008946) Watermelon (developer) 2014-06-09 01:50	Note to self: $music is a gstring / lang lump

(0008947) Edward-san (developer) 2014-06-09 02:35 edited on: 2014-06-09 02:36	You should find some inspiration in function S_ChangeMusic in s_sounds.cpp. There's a specific handle of '$' char in the music name string.

(0009638) Watermelon (developer) 2014-06-22 05:36	'https://bitbucket.org/Torr_Samaho/zandronum-stable/pull-request/53/added-a-new-ccmd-listmusic-which-dumps-the/diff [^]' Works well. I did it the same way that Randy coded it for S_ChangeMusic in terms of determining what to do with GStrings.

(0009655) Torr Samaho (administrator) 2014-06-22 13:56	The implementation itself looks good, there are just two minor style issues: - Don't remove the new lines in the includes. We inherited those from (G)ZDoom and should keep them. If you need to add includes, just add them at the end of the Zandronum specific includes. In this case, this is after "#include "network/nettraffic.h". The next include "g_hub.h" still needs to be separated by a new line. - Code consistency: You use both "(*it)." and "it->". While they should be equivalent, IMO it would be nicer if you decide for one and only use that in such a short code snippet.

(0009666) Watermelon (developer) 2014-06-22 15:44 edited on: 2014-06-22 15:45	All fixed. Now, there was one that used (*it)[0] instead of it->operator[](0), but I assume the former is much more clear and we'd be fine with this exception to the rule? The rest of course are all standardized to -> Also added zan history update since it appears to be finalized. 'https://bitbucket.org/Torr_Samaho/zandronum-stable/pull-request/53/added-a-new-ccmd-listmusic-which-dumps-the/diff [^]'

(0009670) Torr Samaho (administrator) 2014-06-22 16:24	Quote from Watermelon All fixed. Almost ;). With my comment "In this case, this is after "#include "network/nettraffic.h". The next include "g_hub.h" still needs to be separated by a new line." I tried to say that the include has to be in between "network/nettraffic.h" and "g_hub.h". This way it's with the other Zandronum includes. With your changes we would have "Zandronum includes, (G)ZDoom includes, Zandronum includes". Quote from Watermelon Now, there was one that used (*it)[0] instead of it->operator[](0) Sure, that's fine. I mainly wondered about the difference between the two "GetChars", that's the only reason why I noticed the inconsistency.

(0009677) Watermelon (developer) 2014-06-22 18:07	Just so I do it right this time (gotta make this one be the good commit!), is this what you mean #include "network/nettraffic.h" // [CK] For CCMD listmusic #include <set> #include "g_hub.h" #include "gl/gl_functions.h"

(0009678) Torr Samaho (administrator) 2014-06-22 18:10	Exactly. Marking this include is not necessary, since the whole block is already marked as Zandronum includes a few lines above, but you can keep the comment to stress that the STL was introduced here for listmusic.

(0009682) Watermelon (developer) 2014-06-22 21:36	Nice and submitted

(0009714) Torr Samaho (administrator) 2014-06-23 19:01	Compressed and rebased.

(0009828) Arco (updater) 2014-07-03 19:33	Works in V1.3 140703-1806.

Relationships