minecraft-pi-reborn
/
minecraft-pi-reborn
7
11
Fork 13
Code Issues 5 Pull Requests 2 Actions Releases 3 Activity

Add chat history #100

Merged
TheBrokenRail merged 6 commits from bigjango13/minecraft-pi-reborn:master into master 2024-03-19 02:06:04 +00:00
Conversation 10 Commits 6 Files Changed 30 +173 -23
bigjango13 commented 2024-03-08 23:06:03 +00:00
Contributor
Copy Link

I got tired of retyping commands.

This adds more symbols too of course, they make up the bulk of this PR. I also made CUSTOM_VTABLE's _setup_##name##_vtable not static, so stuff like this can be done without a PR.

I got tired of retyping commands. This adds more symbols too of course, they make up the bulk of this PR. I also made `CUSTOM_VTABLE`'s `_setup_##name##_vtable` not static, so stuff like this can be done without a PR.
bigjango13 added 1 commit 2024-03-08 23:06:03 +00:00
3ff24c2a92 Add chat history 
- More symbols too
- Made CUSTOM_VTABLE not static, for modders
TheBrokenRail requested changes 2024-03-09 02:27:37 +00:00
mods/include/mods/text-input-box/TextInputBox.h Outdated
@ -4,3 +4,3 @@
struct TextInputBox {
static TextInputBox *create(const std::string &placeholder = "", const std::string &text = "");
static TextInputBox *create(const std::string &placeholder = "", const std::string &text = "", const std::vector<std::string> *history = NULL);
TheBrokenRail commented 2024-03-09 02:24:07 +00:00
Owner
Copy Link

IMO the history code should be part of chat, not the text input widget.

IMO the history code should be part of chat, not the text input widget.
bigjango13 commented 2024-03-09 17:57:24 +00:00
Author
Contributor
Copy Link

I agree, however that makes input trickier and setting the text harder (unless there was isFocused and setText methods). This also has the benefit of being really easy to add history to any other text input, such as for a modded terminal.

I agree, however that makes input trickier and setting the text harder (unless there was `isFocused` and `setText` methods). This also has the benefit of being really easy to add history to any other text input, such as for a modded terminal.
TheBrokenRail commented 2024-03-10 04:31:10 +00:00
Owner
Copy Link

Then, why not just... add setText and isFocused methods? Realistically chat is the only thing that will ever need this code. And if something else does need it, it can also just be re-implemented really easily.

Then, why not just... add `setText` and `isFocused` methods? Realistically chat is the only thing that will ever need this code. And if something else does need it, it can also just be re-implemented really easily.
bigjango13 commented 2024-03-10 05:02:46 +00:00
Author
Contributor
Copy Link

Right, but what about input?

Right, but what about input?
TheBrokenRail commented 2024-03-10 05:25:32 +00:00
Owner
Copy Link

What do you mean? Just add code to keyPressed in chat/ui.cpp.

What do you mean? Just add code to `keyPressed` in `chat/ui.cpp`.
bigjango13 commented 2024-03-11 02:48:27 +00:00
Author
Contributor
Copy Link

Done 👍

Done 👍
bigjango13 marked this conversation as resolved
mods/src/chat/ui.cpp Outdated
@ -113,6 +119,7 @@ static Screen *create_chat_screen() {
// Init
void _init_chat_ui() {
history = {};
TheBrokenRail commented 2024-03-09 02:25:27 +00:00
Owner
Copy Link

Most of Reborn uses this system to avoid issues with C++ initialization:

static <type> &get_<var>() {
    static <type> <var>;
    return <var>;
}
Most of Reborn uses this system to avoid issues with C++ initialization: ```c++ static <type> &get_<var>() { static <type> <var>; return <var>; } ```
bigjango13 marked this conversation as resolved
symbols/src/level/Biome.def
@ -2,1 +17,4 @@
property int leaf_color = 0x34;
// 64x64, Temp x humidity
static-property-array Biome *map = 0x17c970;
TheBrokenRail commented 2024-03-09 02:26:41 +00:00
Owner
Copy Link

Just to clarify, is this Biome[64][64] or Biome*[64*64]?

Just to clarify, is this `Biome[64][64]` or `Biome*[64*64]`?
bigjango13 commented 2024-03-09 17:57:50 +00:00
Author
Contributor
Copy Link

Biome*[64*64], I'll update the comment to reflect this.

`Biome*[64*64]`, I'll update the comment to reflect this.
bigjango13 marked this conversation as resolved
bigjango13 added 1 commit 2024-03-09 18:02:51 +00:00
a6e0cd8f13 Fix history editing bug 
- Use the `get_<var>` pattern for chat's `history`.
- Make the Biome_map comment clearer
bigjango13 added 1 commit 2024-03-11 02:48:03 +00:00
5e5088e3ef Move chat history into chat 
- `get_death_message` is no longer static
- Fix ItemInHandRenderer_instance symbol
TheBrokenRail reviewed 2024-03-18 22:06:59 +00:00
symbols/src/item/ItemInHandRenderer.def Outdated
@ -6,2 +6,4 @@
method void renderItem(Mob *mob, ItemInstance *item) = 0x4b824;
method void render(float param_1) = 0x4bfcc;
static-property ItemInHandRenderer **instance = 0x137bc0;
TheBrokenRail commented 2024-03-18 22:06:59 +00:00
Owner
Copy Link

This seems wrong. I think this address actually refers to an EntityRenderDispatcher. Which then has a first property of ItemInHandRenderer.

  this_00 = (ItemInHandRenderer *)operator.new(0x70dc);
  ItemInHandRenderer::ItemInHandRenderer(this_00,param_1);
  this->field0_0x0 = this_00;
  ppIVar1 = (ItemInHandRenderer **)EntityRenderDispatcher::getInstance();
  *ppIVar1 = this->field0_0x0;
This seems wrong. I think this address actually refers to an `EntityRenderDispatcher`. Which then has a first property of `ItemInHandRenderer`. ```c++ this_00 = (ItemInHandRenderer *)operator.new(0x70dc); ItemInHandRenderer::ItemInHandRenderer(this_00,param_1); this->field0_0x0 = this_00; ppIVar1 = (ItemInHandRenderer **)EntityRenderDispatcher::getInstance(); *ppIVar1 = this->field0_0x0; ```
TheBrokenRail commented 2024-03-18 22:10:26 +00:00
Owner
Copy Link

Yup, it's EntityRenderer::entityRenderDispatcher.

Yup, it's `EntityRenderer::entityRenderDispatcher`.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-03-18 22:12:09 +00:00
mods/src/chat/ui.cpp Outdated
@ -78,9 +89,32 @@ CUSTOM_VTABLE(chat_screen, Screen) {
ChatScreen *self = (ChatScreen *) super;
if (key == 0x0d && self->chat->isFocused()) {
TheBrokenRail commented 2024-03-18 22:12:09 +00:00
Owner
Copy Link

This should probably be in one big if (self->chat->isFocused()) statement.

This should probably be in one big `if (self->chat->isFocused())` statement.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-03-18 22:12:56 +00:00
mods/src/chat/ui.cpp Outdated
@ -81,1 +92,3 @@
_chat_queue_message(self->chat->getText().c_str());
std::string text = self->chat->getText();
std::vector<std::string> &history = get_history();
if (history.size() == 0 || text != history.back()) {
TheBrokenRail commented 2024-03-18 22:12:56 +00:00
Owner
Copy Link

What's the point of this check?

What's the point of this check?
bigjango13 commented 2024-03-18 22:17:14 +00:00
Author
Contributor
Copy Link

The history.size() == 0 skips the next check, and text != history.back() prevents some duplicates from being added. So if I was using the same command or sending the same message many times it would only be in the history once, but if I said something in-between them it would save both versions, for example:

What's said in chat:
Hello
Hello
Goodbye
Hello

What's in history:
Hello
Goodbye
Hello
The `history.size() == 0` skips the next check, and `text != history.back()` prevents some duplicates from being added. So if I was using the same command or sending the same message many times it would only be in the history once, but if I said something in-between them it would save both versions, for example: ``` What's said in chat: Hello Hello Goodbye Hello What's in history: Hello Goodbye Hello ```
bigjango13 marked this conversation as resolved
TheBrokenRail commented 2024-03-18 22:20:23 +00:00
Owner
Copy Link

This all looks good, but I have one concern.

What if I type something and then accidentally hit up/down, what happens to what I typed.

I'd suggest this system (forgive the pseudo-code):

global std::vector<std::string> history;
ChatScreen {
    std::string history_temp;
    int history_position = -1; // -1 refers to history_temp. 0,1, 2, etc, refer to entries in history.
}

// When Pressing Up/Down
1. Save text in `TextInputBox` to either `history[history_position]` or `history_temp`
2. Update `history_position`
3. Update text in `TextInputBox` to either `history[history_position]` or `history_temp`

// When Submitting
if (history_position == -1) {
    history.push_back(text_box.getText());
} else {
    history[history_position] = text_box.getText();
}

I hope that makes sense.

This all looks good, but I have one concern. What if I type something and then accidentally hit up/down, what happens to what I typed. I'd suggest this system (forgive the pseudo-code): ```cpp global std::vector<std::string> history; ChatScreen { std::string history_temp; int history_position = -1; // -1 refers to history_temp. 0,1, 2, etc, refer to entries in history. } // When Pressing Up/Down 1. Save text in `TextInputBox` to either `history[history_position]` or `history_temp` 2. Update `history_position` 3. Update text in `TextInputBox` to either `history[history_position]` or `history_temp` // When Submitting if (history_position == -1) { history.push_back(text_box.getText()); } else { history[history_position] = text_box.getText(); } ``` I hope that makes sense.
bigjango13 commented 2024-03-18 22:21:55 +00:00
Author
Contributor
Copy Link

I hope that makes sense.

Yep, will do 👍

> I hope that makes sense. Yep, will do 👍
bigjango13 commented 2024-03-18 22:30:53 +00:00
Author
Contributor
Copy Link

To be clear, you want it to modify the history? So if I run /gamemode 0, then go back and edit it to /gamemode 1 and hit enter, the history should just be /gamemode 1?

To be clear, you want it to modify the history? So if I run `/gamemode 0`, then go back and edit it to `/gamemode 1` and hit enter, the history should just be `/gamemode 1`?
TheBrokenRail commented 2024-03-18 22:42:19 +00:00
Owner
Copy Link

Hmm, that could be a problem.

Maybe when ChatScreen is created, it copies history into it's own property (history_local?). That gets modified when you hit up/down. But when you hit enter, what you typed gets pushed to the global history (unless it is equal to what was pushed last).

So history itself is never modified, only appended to. But if type something, then hit up/down, it won't just be thrown away.

Maybe:

global std::vector<std::string> history;
ChatScreen {
    std::vector<std::string> history_local;
    int history_position;
}

// When Constructing ChatScreen
for (std::string entry : history) {
    history_local.push_back(entry);
}
history.push_back("");
history_position = history.size() - 1;

// When Pressing Up/Down
1. Save text in `TextInputBox` to either `history_local[history_position]`
2. Update `history_position`
3. Update text in `TextInputBox` to either `history_local[history_position]`

// When Submitting
std::string text = text_box.getText();
if (history.size() == 0 || history[history.size() - 1] != text) {
    history.push_back(text);
}
Hmm, that could be a problem. Maybe when `ChatScreen` is created, it copies `history` into it's own property (`history_local`?). That gets modified when you hit up/down. But when you hit enter, what you typed gets pushed to the global `history` (unless it is equal to what was pushed last). So `history` itself is never modified, only appended to. But if type something, then hit up/down, it won't just be thrown away. Maybe: ```cpp global std::vector<std::string> history; ChatScreen { std::vector<std::string> history_local; int history_position; } // When Constructing ChatScreen for (std::string entry : history) { history_local.push_back(entry); } history.push_back(""); history_position = history.size() - 1; // When Pressing Up/Down 1. Save text in `TextInputBox` to either `history_local[history_position]` 2. Update `history_position` 3. Update text in `TextInputBox` to either `history_local[history_position]` // When Submitting std::string text = text_box.getText(); if (history.size() == 0 || history[history.size() - 1] != text) { history.push_back(text); } ```
bigjango13 commented 2024-03-18 22:46:33 +00:00
Author
Contributor
Copy Link

FWIW, Minecraft only saves the first entry, anything in history is refreshed when up/down is used. This seems a bit complex compared to something like that.

FWIW, Minecraft only saves the first entry, anything in history is refreshed when up/down is used. This seems a bit complex compared to something like that.
TheBrokenRail commented 2024-03-18 22:47:18 +00:00
Owner
Copy Link

I'm basing this off GNOME Terminal (or Bash, I'm not sure who's responsible for history), which IMO, has a much superior history function to Minecraft.

I'm basing this off GNOME Terminal (or Bash, I'm not sure who's responsible for history), which IMO, has a much superior history function to Minecraft.
bigjango13 commented 2024-03-18 22:48:48 +00:00
Author
Contributor
Copy Link

Alright, will do 👍

Alright, will do :+1:
bigjango13 added 1 commit 2024-03-18 23:45:22 +00:00
bigjango13 commented 2024-03-18 23:45:55 +00:00
Author
Contributor
Copy Link

Done.

Done.
TheBrokenRail reviewed 2024-03-18 23:47:43 +00:00
mods/include/mods/title-screen/title-screen.h
@ -3,4 +3,6 @@
#include <string>
#include <vector>
extern "C" {
TheBrokenRail commented 2024-03-18 23:47:43 +00:00
Owner
Copy Link

Why? This seems really useless. Considering it needs std::vector... which only exists on C++.

Why? This seems really useless. Considering it needs `std::vector`... which only exists on C++.
bigjango13 commented 2024-03-18 23:49:53 +00:00
Author
Contributor
Copy Link

Of course, but it's needed to work with the normal HOOK macro.

Of course, but it's needed to work with the normal `HOOK` macro.
bigjango13 commented 2024-03-18 23:53:07 +00:00
Author
Contributor
Copy Link

Some cursedness could be done with typeid, but that would require changing the HOOK macro

Some cursedness could be done with `typeid`, but that would require changing the `HOOK` macro
TheBrokenRail commented 2024-03-18 23:59:43 +00:00
Owner
Copy Link

What is the issue with HOOK and non-extern"C"-functions?

What is the issue with `HOOK` and non-`extern"C"`-functions?
bigjango13 commented 2024-03-19 00:06:24 +00:00
Author
Contributor
Copy Link

dlsym(RTLD_NEXT, #name); assumes that the name is just "name", which isn't true for mangled names. It can intercept it fine, just not call the original.

`dlsym(RTLD_NEXT, #name);` assumes that the name is just "name", which isn't true for mangled names. It can intercept it fine, just not call the original.
TheBrokenRail commented 2024-03-19 00:54:46 +00:00
Owner
Copy Link

Well, that's annoying. I don't want to mess with typeid, so I guess this is the best solution.

Well, that's annoying. I don't want to mess with `typeid`, so I guess this is the best solution.
TheBrokenRail marked this conversation as resolved
TheBrokenRail reviewed 2024-03-18 23:55:49 +00:00
mods/src/chat/ui.cpp
@ -82,0 +103,4 @@
int old_pos = self->history_pos;
self->history_pos -= 1;
if (self->history_pos < 0) self->history_pos = local_history.size() - 1;
if (old_pos != self->history_pos) {
TheBrokenRail commented 2024-03-18 23:55:49 +00:00
Owner
Copy Link

Is this check needed? Because if old_pos == self->history_pos, wouldn't that just call setText with the current text, doing nothing?

Is this check needed? Because if `old_pos == self->history_pos`, wouldn't that just call `setText` with the current text, doing nothing?
bigjango13 commented 2024-03-18 23:58:58 +00:00
Author
Contributor
Copy Link

Yep, that's left over.

Yep, that's left over.
bigjango13 marked this conversation as resolved
bigjango13 added 1 commit 2024-03-18 23:58:57 +00:00
bigjango13 commented 2024-03-19 00:55:44 +00:00
Author
Contributor
Copy Link

Hold on I'm going to sneak on last symbol in ;)

Hold on I'm going to sneak on last symbol in ;)
bigjango13 added 1 commit 2024-03-19 01:02:20 +00:00
CI / Build (AMD64, Server) (push) Successful in 12m25s Details
CI / Build (AMD64, Client) (push) Successful in 12m41s Details
CI / Build (ARM64, Server) (push) Successful in 12m25s Details
CI / Build (ARM64, Client) (push) Successful in 12m56s Details
CI / Build (ARMHF, Server) (push) Successful in 8m51s Details
CI / Build (ARMHF, Client) (push) Successful in 11m57s Details
CI / Test (Client) (push) Successful in 14m36s Details
CI / Test (Server) (push) Successful in 12m32s Details
CI / Release (push) Has been skipped Details
CI / Build Example Mods (push) Failing after 7m21s Details
76a66a0ba5 Sneak in Level_setTileEntity
bigjango13 commented 2024-03-19 01:02:48 +00:00
Author
Contributor
Copy Link

Alright, I'm ready if you are.

Alright, I'm ready if you are.
TheBrokenRail approved these changes 2024-03-19 02:05:56 +00:00
TheBrokenRail merged commit 76a66a0ba5 into master 2024-03-19 02:06:04 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
TheBrokenRail
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: minecraft-pi-reborn/minecraft-pi-reborn#100
No description provided.
Delete Branch "bigjango13/minecraft-pi-reborn:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?