New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented books #3877

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Seadragon91
Contributor

Seadragon91 commented Jul 30, 2017

Done:

  • Class named cBookContent exported to lua - Is the name okay? Anyone has a better one?
  • Documentation in APIDoc
  • NBT: Loading / saving
  • Check for channels: MC|BEdit and MC|BSign
  • Player data: Loading / saving
  • Hooks: HOOK_PLAYER_EDITING_BOOK and HOOK_PLAYER_EDITED_BOOK
  • Sending of the changed item
  • NBT content of books is now read, if creative inventory is opened

The book can be edited and opened in clients 1.9 and above.


cInventory & inv = Player->GetInventory();
inv.SetHotbarSlot(inv.GetEquippedSlotNum(), BookItem);
SendWholeInventory(*Player->GetWindow()); // TODO: Use SendSlot

This comment has been minimized.

@Seadragon91

Seadragon91 Jul 30, 2017

Contributor

I see that there is a method named SendSlot in cWindow, but it requires a slot area.

This comment has been minimized.

@madmaxoft

madmaxoft Jul 30, 2017

Member

I think this piece of code should be factored out into the cClientHandle or cPlayer class.

Then, the cWindow class could probably implement a new method, SendSlot, that would take a Player and SlotNum parameters, find the correct cSlotArea and call its SendSlot.

This comment has been minimized.

@Seadragon91

Seadragon91 Jul 31, 2017

Contributor

Found this one... Player.GetInventory().SendEquippedSlot();

Should the handling of the books moved into cClientHandle / cPlayer?

{
Pages.push_back(Page);
}
lua_pop(L, 1);

This comment has been minimized.

@Seadragon91

Seadragon91 Jul 31, 2017

Contributor

Is it possible to usecStackTable::ForEachArrayElement here? If yes how?
Whole code

Edit: Or a other better way to read the table

This comment has been minimized.

@madmaxoft

madmaxoft Jul 31, 2017

Member

Yes, it should be possible, look at the LuaChunkStay example. Just push each value into a vector.

This comment has been minimized.

@Seadragon91

Seadragon91 Jul 31, 2017

Contributor

I tried to use StackTable

cLuaState::cStackTablePtr Table;
L.GetStackValue(2, Table);
Table->ForEachArrayElement([=](cLuaState & a_LuaState, int a_Index) -> bool
	{
		LOG("Type = %s", a_LuaState.GetTypeText(a_Index));
		return false;
	}
);

This prints userdata, table and string. I expected three strings.

This comment has been minimized.

@madmaxoft

madmaxoft Jul 31, 2017

Member

Do you have the code on a branch so that I can give it a try?

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 1, 2017

Contributor

Looking at LuaChunkStay again, I would have to use lua_rawgeti then it should work. Another problem I see is that I cannot add the string value to a vector that is outside of the function, all variables outside of the function are marked const.

Can I keep the current code?

This comment has been minimized.

@madmaxoft

madmaxoft Aug 1, 2017

Member

The a_Index parameter to the callback specifies the array index that is being processed. Get the value by using a_LuaState.GetStackValue(-1, txt)

@Seadragon91 Seadragon91 force-pushed the books branch 3 times, most recently from c3e624b to d454b46 Aug 1, 2017

@Seadragon91

This comment has been minimized.

Contributor

Seadragon91 commented Aug 3, 2017

Fixed book content lost on open creative inventory. Nothing left on the todo.

@Seadragon91 Seadragon91 changed the title from Started on adding books to Implemented books Aug 3, 2017

@@ -6932,14 +6932,19 @@ These ItemGrids are available in the API and can be manipulated by the plugins,
},
Variables =
{
m_BookContent =
{
Type = "{{cBookContent|cBookContent}}",

This comment has been minimized.

@madmaxoft

madmaxoft Aug 3, 2017

Member

No need to specify the string twice. APIDump can understand both of these:

  • {{link}}
  • {{link|display string}}

Additionally, it automatically linkifies types it understands from the Type member, so the only thing you actually need to do is Type = "cBookContent",

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 3, 2017

Contributor

This Type = "cBookContent" doesn't work for member variables (no link). have to use {{cBookContent}}. Will look into APIDump what there is wrong.

Params =
{
{ Name = "Player", Type = "{{cPlayer}}", Notes = "The player that is editing the book" },
{ Name = "BookContent", Type = "{{cBookContent}}", Notes = "The class that contains the current info of the book" },

This comment has been minimized.

@madmaxoft

madmaxoft Aug 3, 2017

Member

Is this before or after the change? Can we get both, somehow? Ideally so that a plugin can modify the new content.

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 3, 2017

Contributor

The content of the book can be modified.

BookContent->AddPage(Page);
return false;
}
);

This comment has been minimized.

@madmaxoft

madmaxoft Aug 3, 2017

Member

This seems to add pages, instead of setting them. So calling this multiple times will not replace, but instead append. Intended?

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 3, 2017

Contributor

Changed, thanks. Pages are now cleared before.

@@ -74,6 +74,8 @@ class cPlugin
virtual bool OnPlayerBrokenBlock (cPlayer & a_Player, int a_BlockX, int a_BlockY, int a_BlockZ, char a_BlockFace, BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta) = 0;
virtual bool OnPlayerDestroyed (cPlayer & a_Player) = 0;
virtual bool OnPlayerEating (cPlayer & a_Player) = 0;
virtual bool OnPlayerEditedBook (cPlayer & a_Player, cBookContent & a_BookContent, bool a_IsSigned) = 0;

This comment has been minimized.

@madmaxoft

madmaxoft Aug 3, 2017

Member

const content?

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 3, 2017

Contributor

Changed.

@@ -1053,6 +1071,8 @@ const char * cPluginLua::GetHookFnName(int a_HookType)
case cPluginManager::HOOK_PLAYER_BREAKING_BLOCK: return "OnPlayerBreakingBlock";
case cPluginManager::HOOK_PLAYER_BROKEN_BLOCK: return "OnPlayerBrokenBlock";
case cPluginManager::HOOK_PLAYER_EATING: return "OnPlayerEating";
case cPluginManager::HOOK_PLAYER_EDITED_BOOK: return "OnPlayerEditedBook";
case cPluginManager::HOOK_PLAYER_EDITING_BOOK: return "OnPlayerSigningdBook";

This comment has been minimized.

@madmaxoft

madmaxoft Aug 3, 2017

Member

Name mismatch.

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 3, 2017

Contributor

Corrected

@@ -105,7 +105,7 @@ void cNBTChunkSerializer::AddItem(const cItem & a_Item, int a_Slot, const AStrin
// Write the tag compound (for enchantment, firework, custom name and repair cost):
if (
(!a_Item.m_Enchantments.IsEmpty()) ||
((a_Item.m_ItemType == E_ITEM_FIREWORK_ROCKET) || (a_Item.m_ItemType == E_ITEM_FIREWORK_STAR)) ||
((a_Item.m_ItemType == E_ITEM_FIREWORK_ROCKET) || (a_Item.m_ItemType == E_ITEM_FIREWORK_STAR) || (a_Item.m_ItemType == E_ITEM_WRITTEN_BOOK)) || (a_Item.m_ItemType == E_ITEM_BOOK_AND_QUILL) ||

This comment has been minimized.

@madmaxoft

madmaxoft Aug 3, 2017

Member

Split this into two lines

@@ -74,6 +74,8 @@ class cPlugin
virtual bool OnPlayerBrokenBlock (cPlayer & a_Player, int a_BlockX, int a_BlockY, int a_BlockZ, char a_BlockFace, BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta) = 0;
virtual bool OnPlayerDestroyed (cPlayer & a_Player) = 0;
virtual bool OnPlayerEating (cPlayer & a_Player) = 0;
virtual bool OnPlayerEditedBook (cPlayer & a_Player, const cBookContent & a_BookContent, bool a_IsSigned) = 0;
virtual bool OnPlayerEditingBook (cPlayer & a_Player, const cBookContent & a_BookContent, bool a_IsSigned) = 0;

This comment has been minimized.

@madmaxoft

madmaxoft Aug 3, 2017

Member

Not exactly what I had in mind - now it expects the plugin cannot modify the book content.
I was looking for something like:

virtual bool OnPlayerEditingBook        (cPlayer & a_Player, const cBookContent & a_OriginalContent, cBookContent & a_NewContent /* Editable */, bool a_IsSigned) = 0;

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 3, 2017

Contributor

I reverted the const change for testing. With the current code I can access the content of the book and modify it in the hook.
Is there a specific problem why a new variable should be used for return?

This comment has been minimized.

@madmaxoft

madmaxoft Aug 3, 2017

Member

I'd like the plugin to be able to see the original contents of the book (before the player even started editing it). That's all.

At least I assume that a player can edit a book multiple times, can't they?

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 3, 2017

Contributor

That works with the current code. I tested it and I can get the current content of the book in in the hooks and can modify it. It work's fine. Do you want test it? A example plugin?

This comment has been minimized.

@madmaxoft

madmaxoft Aug 3, 2017

Member

How do you know where to get the contents - from which item?

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 4, 2017

Contributor

It's possible to get the book content over the equipped item. Is that enough for you? If yes I will add a note to both hooks description.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 4, 2017

Member

So it's not possible to edit a book not currently equipped?

Anyway, just why not provide the old contents to the hook nevertheless? It's minimal amount of work and it will simplify the lives of plugin devs.

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 4, 2017

Contributor

Added a new param and changed the names. I just found another problem. If a player signs a book in vanilla the text is saved into a json string.
This is now the same here, if the book is signed. The pages in OriginalContent are strings, in NewContent they are json strings.

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 17, 2017

Contributor

Storing the pages as json is also not good. As a json is made of multiple parts, e.g. color, formatting, extra parts. Getting the text from it is diffcult. Whats about using cCompositeChat for the pages? The text can be extracted with ExtractText and CreateJsonString can return a json string.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 17, 2017

Member

Sounds good.

@Seadragon91 Seadragon91 force-pushed the books branch 2 times, most recently from 0600217 to 5d8d1f5 Aug 4, 2017

@@ -141,6 +141,7 @@ set(BINDING_DEPENDENCIES
../Vector3.h
../WebAdmin.h
../World.h
../BookContent.h

This comment has been minimized.

@madmaxoft

madmaxoft Aug 7, 2017

Member

Alpha-sort

@@ -151,6 +152,7 @@ SET (HDRS
VoronoiMap.h
WebAdmin.h
World.h
BookContent.h

This comment has been minimized.

@madmaxoft

madmaxoft Aug 7, 2017

Member

Alpha-sort both.

{
a_Pkt.WriteBEInt8(0);
return;
}

if ((ItemType == E_ITEM_BOOK_AND_QUILL) && (a_Item.m_BookContent.GetPages().size() == 0))

This comment has been minimized.

@madmaxoft

madmaxoft Aug 7, 2017

Member

Use .empty instead of .size() to check emptiness.

cBookContent =
{
Desc = [[
This class contains the information for a signed or writeable book: The author, title and the pages. A page of the writeable book is a simple string. For a signed book it can be a json string. For the json string use {{cCompositeChat}} to create a page and then the function {{cCompositeChat#CreateJsonString_1|CreateJsonString}} to get the json string.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 7, 2017

Member

How about adding an overload that accepts a cCompositeChat and performs the conversion internally?

This comment has been minimized.

@madmaxoft

madmaxoft Aug 7, 2017

Member

Or even using cCompositeChat exclusively for all setters ? (But getters should still return a string, in case there's some extra json that cCompositeChat wouldn't handle)

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 7, 2017

Contributor

I will add an overload, because a json string will only work for signed books.

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 7, 2017

Contributor

Or should I accept for both book types a json string. Then internally if the book is not signed get the string from key text and add this to the page?

This comment has been minimized.

@madmaxoft

madmaxoft Aug 7, 2017

Member

Ideally accept and store json for both and then in the protocol handler convert whatever is not supported into a supported form. So that if MC decides to support json in the future, we don't have trouble upgrading stuff.

Type = "table",
},
},
Notes = "Set the pages of the book",

This comment has been minimized.

@madmaxoft

madmaxoft Aug 7, 2017

Member

Sets all the pages of the book

How about adding a SetPage(PageNum, Text), and appropriate GetPage(PageNum)

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 7, 2017

Contributor

Zero-based?

This comment has been minimized.

@madmaxoft

madmaxoft Aug 7, 2017

Member

Not sure. Lua traditionally uses 1-based, but I think we already have a few things exported 0-based.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 7, 2017

Member

Make it 1-based. It means manual bindings, but it's more consistent for the plugin devs. The C++ side should be 0-based, of course.

@@ -104,7 +104,8 @@ void cNBTChunkSerializer::AddItem(const cItem & a_Item, int a_Slot, const AStrin
// Write the tag compound (for enchantment, firework, custom name and repair cost):
if (
(!a_Item.m_Enchantments.IsEmpty()) ||
((a_Item.m_ItemType == E_ITEM_FIREWORK_ROCKET) || (a_Item.m_ItemType == E_ITEM_FIREWORK_STAR)) ||
((a_Item.m_ItemType == E_ITEM_FIREWORK_ROCKET) || (a_Item.m_ItemType == E_ITEM_FIREWORK_STAR) ||

This comment has been minimized.

@lkolbly

lkolbly Aug 12, 2017

Contributor

Since this condition is a string of ORs, I think the double opening parentheses can be removed. (grouping the FIREWORK_ROCKET, FIREWORK_STAR, and WRITTEN_BOOK conditionals)

This comment has been minimized.

@madmaxoft

madmaxoft Aug 17, 2017

Member

At the very least the double-opening-parenthesis is mismatched, closing in between the two conditions for books. I'd vote for removing it as well (but keep the firework stuff on one line and the book stuff on another line.

@Seadragon91

This comment has been minimized.

Contributor

Seadragon91 commented Aug 27, 2017

Using cCompositeChat internal for the pages is also not good. It's still now strings, but the methods accepts a cCompositeChat and if the book is signed it stores as json string or if not as simple string.
For a signed book vanilla allows the a page to be a simple string or a json string.

Edit: I see that appveyor always builds branches that are in cuberite. There is a setting that wouldn't build anymore branches that are listed on cuberite: "General" tab of project settings - Do not build feature branches with open Pull Requests. Can I change it?

Changes done:
- Added overload for pages with cCompositeChat
- Added methods GetPage, AddPage, SetPage and changed SetPages
- Updated APIDoc
Type = "string",
},
},
Notes = "Set's the page at the given index. Note: one-based",

This comment has been minimized.

@madmaxoft
Type = "cCompositeChat",
},
},
Notes = "Set's the page at the given index. For signed book saves it as json string, otherwise as simple string. Note: one-based",

This comment has been minimized.

@madmaxoft
CalledWhen = "A player has edited a book.",
DefaultFnName = "OnPlayerEditedBook", -- also used as pagename
Desc = [[
This hook is called whenever a {{cPlayer|player}} has edited a book.

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

whenever -> after

Desc = [[
This hook is called whenever a {{cPlayer|player}} is editing a book.
See also the {{OnPlayerEditingBook|HOOK_PLAYER_EDITED_BOOK}} hook for a similar hook, is called when a
player has edited a book.

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

Add the note that the plugin may override the player edits by changing the NewContent parameter

Params =
{
{ Name = "Player", Type = "cPlayer", Notes = "The player that edited the book" },
{ Name = "NewContent", Type = "cBookContent", Notes = "Contains the new content of the book" },

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

Should have the OldContent parameter, too.

}
else
{
BookContent->AddPage(CompositeChat->ExtractText());

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

This will remove all formatting that could have been applied to the cCompositeChat. We might want to keep it, using the old-style formatting codes, or is that not possible?

This comment has been minimized.

@Seadragon91

Seadragon91 Sep 10, 2017

Contributor

Ah well this is a problem. What do you mean with old style?

This comment has been minimized.

@madmaxoft

madmaxoft Sep 19, 2017

Member

Old-style formatting uses § in the text to switch colors and styles. The new formatting uses JSON for the different parts.

tolua_function(tolua_S, "GetPage", tolua_cBookContent_GetPage);
tolua_function(tolua_S, "GetPages", tolua_cBookContent_GetPages);
tolua_function(tolua_S, "SetPage", tolua_cBookContent_SetPage);
tolua_function(tolua_S, "SetPages", tolua_cBookContent_SetPages);

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

Align the parameters vertically

@@ -961,14 +999,14 @@ bool cPluginManager::CallHookPlayerJoined(cPlayer & a_Player)



bool cPluginManager::CallHookPlayerLeftClick(cPlayer & a_Player, int a_BlockX, int a_BlockY, int a_BlockZ, char a_BlockFace, char a_Status)
bool cPluginManager::CallHookPlayerOpeningWindow(cPlayer & a_Player, cWindow & a_Window)

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

Why the move? The functions were kinda alpha-sorted before, now it doesn't make sense

This comment has been minimized.

@Seadragon91

Seadragon91 Sep 1, 2017

Contributor

I moved the two functions, because I had old hook / function names. Not sure why the change looks that strange ...

This comment has been minimized.

@madmaxoft

madmaxoft Sep 7, 2017

Member

Just swap the functions back into the order they were in.

/** Returns a ref to the vector. Used in ManualBindings and for saving the book */
const std::vector<AString> & GetPages(void) const { return m_Pages; }

/** Read the book content from nbt. The boolean a_SaveAsJson is optional. If the book is signed, the text should be in a json string */

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

Doesn't explain what the SaveAsJson does nor when exactly it should be used

const std::vector<AString> & GetPages(void) const { return m_Pages; }

/** Read the book content from nbt. The boolean a_SaveAsJson is optional. If the book is signed, the text should be in a json string */
static void ParseFromNBT(int TagTag, cBookContent & a_BookContent, const cParsedNBT & a_NBT, bool a_SaveAsJson = false);

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

Prefer to return-by-value, rather than pass-by-out-param

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment