Skip to content

Conversation

@robalni
Copy link
Contributor

@robalni robalni commented Jul 26, 2020

This makes it possible to execute the start command like this:

/start castle dm ffa duel

Most modes have an alias, like dm for deathmatch. The old way of using
this command with numbers still works, but the difference is that the
command takes strings instead of numbers for mode and muts. This
probably doesn't make any difference since types are implicitly
converted in cubescript.

Robert Alm Nilsson added 2 commits July 26, 2020 16:27
This makes it possible to execute the start command like this:

    /start castle dm ffa duel

Most modes have an alias, like dm for deathmatch.  The old way of using
this command with numbers still works, but the difference is that the
command takes strings instead of numbers for mode and muts.  This
probably doesn't make any difference since types are implicitly
converted in cubescript.
@MoonPadUSer MoonPadUSer requested review from TheAssassin and removed request for TheAssassin July 30, 2020 22:03
@TheAssassin
Copy link
Member

TheAssassin commented Jul 30, 2020

How about (as an alternative) using the existing commands like /dm? They could be extended to support string arguments.

@MoonPadUSer
Copy link
Contributor

maybe instead call it vote or suggestsince that seems more intuitive and describes its functionality better

@TheAssassin
Copy link
Member

I'm not so sure about my initial assumption any more. Looks like this new command does not use the voting system. But... maybe it should? I would enjoy being able to vote my favorite modes properly using the commands.

@robalni
Copy link
Contributor Author

robalni commented Jul 31, 2020

How about (as an alternative) using the existing commands like /dm? They could be extended to support string arguments.

They use the start command so making start support strings is a step towards making dm support strings.

Robert Alm Nilsson added 3 commits July 31, 2020 13:20
This allows mixing numeric and names.
This makes it possible to use those commands and easily specify
additional mutators as argument using strings.
@robalni
Copy link
Contributor Author

robalni commented Jul 31, 2020

In these latest commits I have made it possible to use strings with dm and similar commands. The only problem is that you have to specify the mutators in one argument to those commands (like "ffa-basic" instead of "ffa basic") because these commands are defined as Cubescript aliases and I don't know of any way to access a list of all argument in Cubescript (like $* or $@ in bash)

@TheAssassin
Copy link
Member

I'm sure @MoonPadUSer can assist with that.

@MoonPadUSer
Copy link
Contributor

@robalni you can get the number of arguments in cubescript by using $numargs IIRC

@robalni
Copy link
Contributor Author

robalni commented Aug 1, 2020

I know how to get the number of arguments. What I tried to do was to get a list of the arguments in a short way.

@MoonPadUSer
Copy link
Contributor

You mean smth else than doing

loop i $numargs [
    $[arg@[i]]
]

?

@robalni
Copy link
Contributor Author

robalni commented Aug 1, 2020

The best would be something to put instead of $arg2 in these definitions to keep them short.

deathmatch      = [ start $arg1 $modeidxdeathmatch $arg2 ]

@TheAssassin
Copy link
Member

Keeping them short is not as much of a priority as being able to use them without - as separators, I would say.

@robalni
Copy link
Contributor Author

robalni commented Aug 1, 2020

I made a command that returns a list of all arguments. I don't understand cubescript very well but I got it to work after a bit of brute force. :)

@MoonPadUSer
Copy link
Contributor

I just checked the cubescript code and it looks ok, no complaints from my side

{
nextmode = atoi(mode_str);
}
else if(mode_str && *mode_str)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer proper pointer comparisons. Please replace with e.g.,

Suggested change
else if(mode_str && *mode_str)
else if(mode_str != nullptr && *mode_str != '\0')

or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually try to follow the existing style when I write code. What are we trying to do here? Are we trying to change the style of the code? Then what's the new style that we should follow?

{"bb", G_BOMBER},
{"race", G_RACE},
};
static const struct {const char *name; int val; int modes;} mut_names[] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a struct array. Come on, we've got the power of C++ 11/14. Why not make it an std::set<std::string, std::pair<size_t, size_t>> or so? There is really no reason to use anonymous structs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a struct array? I like it when code is simple and easy to understand. Why should I put a wrapper around my char array (called std::string) that might allocate it on the heap and do some other undefined things when I don't need that? And why should I do similar thing with the whole array? It might even be slower. I never add things to these arrays either so they don't need to grow. They are just static data.

else if(mode_str && *mode_str)
{
bool found_one = false;
for(auto possible_mode : mode_names)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be const auto or even const auto&.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a proper map will also eliminate the need to implement a custom linear search. If you wanted, you could've even used std::find_if here if you used an std::vector.

}
//ICOMMAND(0, sendmap, "", (), sendmap());

static void startcmd(char *map, char *mode_str, char *muts_str)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could likely use std::string instead of these char arrays.

Copy link
Contributor Author

@robalni robalni Aug 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could but that doesn't give me any benefit. The only change it would make is that I would have to type ".c_str()" more inside the function. (ok maybe I can skip a strcmp)

@TheAssassin TheAssassin added this to the 1.7.0 milestone Dec 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants