In-game console #33
In-game console #33
Conversation
|
Can we consider squashing multiple commits into one before merging? Generally a good rule I have come across is "Each commit should be a functioning piece of software", so if the feature is not functional/complete unless all 10 commits are in, then they should be represented as one commit with a good description. |
| { | ||
| protected T* _value; | ||
|
|
||
| public ref T Value => *_value; |
There was a problem hiding this comment.
This seems a bit dangerous if the value is null. We should have null protections or require that a CVar be non-null
There was a problem hiding this comment.
You shouldn't be able to register a null CVar. The GameConsole.RegisterVariable takes ref so I don't think there is way to pass null.
Engine/src/Console/CVarUtil.bf
Outdated
| case "true", "True", "TRUE", "1": | ||
| result = true; | ||
| case "false", "False", "FALSE", "0": |
There was a problem hiding this comment.
Instead of checking for the different spellings, why not convert these to lowercase and then just check for ["false", 0] and ["true", 1]?
Engine/src/Console/CVarUtil.bf
Outdated
| case "false", "False", "FALSE", "0": | ||
| result = false; | ||
| default: | ||
| return false; |
There was a problem hiding this comment.
In most languages, all non-zero numbers are truthy. Should we do the same here rather than default to false?
There was a problem hiding this comment.
return false in this contex means that we failed to parse the value and value wasn't changed. But I can add that if the value is number other than zero it will change to true.
| return false; | ||
| } | ||
|
|
||
| public static int32 GetValueInt32(bool* val) |
There was a problem hiding this comment.
All of these GetValue* functions assume the pointers are not null. Are we okay setting the precedent that we just assume all these values are non-null?
| { | ||
| class CommandsHistory | ||
| { | ||
| private String[] _history ~ DeleteContainerAndItems!(_); |
There was a problem hiding this comment.
Why not just use a List since this is expandable?
There was a problem hiding this comment.
Okay, I see later on that we have a max history length. The array is great then for performance reasons, but we should also still consider allowing the history length to change at runtime and handle the copying of old history items to the new container if need be.
There was a problem hiding this comment.
Will add Resize method and make maximum size configurable through cvar.
| _addPos++; | ||
| _count++; | ||
| if (_addPos == _history.Count) | ||
| _addPos = 0; | ||
|
|
||
| if (_count > _history.Count) | ||
| _count--; | ||
|
|
||
| let item = _history[_addPos]; | ||
| item.Set(line); |
There was a problem hiding this comment.
If a list is used, the history can still wrap around like this, while also allowing a dynamic change in how big the history should be at runtime.
| public StringView At(int index) | ||
| { | ||
| // index 0 = empty string | ||
| // index 1 = last addeded entry |
There was a problem hiding this comment.
Change addeded to added
Fusioon
force-pushed
the
in-game-console
branch
3 times, most recently
from
913851c
to
5b32d9b
Compare
Fusioon
force-pushed
the
in-game-console
branch
from
5b32d9b
to
6940028
Compare
Overview
System allowing easy registration of commands and variables. Commands are delegates which will have arguments passed into them as
StringView strArgscontaining whole argument andSpan<StringView> argscontaining arguments separated by space or in quotes. Variables support most common types (bool, int, float, enums, String). Each variable type can have their value set by string. Support for configuration variables meaning that when configuration file is parsed, it saves all values contained in this file and when variable is registered with flagCVarFlags.Configvalue of this variable will immediately change to match the value that was in the config file.TODO