Skip to content
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

Add dynamic media without loading from disk #12757

Closed
wants to merge 11 commits into from
Closed

Add dynamic media without loading from disk #12757

wants to merge 11 commits into from

Conversation

GoodClover
Copy link
Contributor

@GoodClover GoodClover commented Sep 11, 2022

Ready for review. Implements & closes #12755.

This PR simply adds the ability to send ephemeral media from a file already in memory as a Lua string. Rather than having to save it to disk, have the engine read it, and then delete it from disk again.

updated doc for dynamic_add_media in lua_api.txt

minetest.dynamic_add_media({
  filename = "foo.png",
  data = "<raw-png-file>",
  -- data implies (requires) ephemeral=true
}, function() end)

As a side-effect, this allows the following, which will read bar.png from disk but send it to the client as foo.png. You could add an error for this, but why remove a free feature that harms no-one?

minetest.dynamic_add_media({
  filename = "foo.png",
  filepath = modpath.."/bar.png",
})

To do

  • Implement the thing.
  • Error checking on table passed to dynamic_add_media.
  • Documentation.
  • Feature flag?

How to test

  1. Download the mod test.zip
  2. Check out init.lua, it's not long at all.
  3. Load up a world, wait 1 second.
  4. Observe an upright_sprite added at your location, both textures working.
  5. =^)

@GoodClover
Copy link
Contributor Author

GoodClover commented Sep 11, 2022

This is my first time writing C++, so for now there's quite a bit of code duplication (two functions are copies of others with small changes) and I've been avoiding figuring out what combination of &*s I should have in arguments (passing by what I think is value presently).

But it works! Just the code needs tidying.

@GoodClover GoodClover marked this pull request as ready for review September 11, 2022 14:21
@GoodClover GoodClover marked this pull request as draft September 11, 2022 14:22
@GoodClover
Copy link
Contributor Author

Shoot, misclick… checks thing disappeared and placed the button right below my cursor as I clicked, thanks GitHub.

@sfan5 sfan5 added @ Script API Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Sep 11, 2022
src/server.cpp Outdated Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved

if (lua_istable(L, 1)) {
getstringfield(L, 1, "filepath", filepath);
getstringfield(L, 1, "to_player", to_player);
getboolfield(L, 1, "ephemeral", ephemeral);
getstringfield(L, 1, "filename", filename);
fromdata = getstringfield(L, 1, "data", filedata);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of copying here, you could use a const char * to the lua string, and the lua string's size, and pass that around.
Then you only have to memcpy the filedata once: When you put the string into the network packet.

In such cases I wish we had string views...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std::string_view is C++17 :(
Is changing everything to a const char * (+keeping track of length) really the only way?
Would require re-working addMediaFile, as that expects a std::string& which it can assign to, managing that's above my C++ knowledge.

Copy link
Member

Choose a reason for hiding this comment

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

If it's too much work, just keep it at copying into std::string.

@savilli
Copy link
Contributor

savilli commented Sep 13, 2022

Doesn't it still require to generate a unique filename? For me it looks like it doesn't solve much. If you already generated a filename, you can just save your content to that file.

@GoodClover
Copy link
Contributor Author

GoodClover commented Sep 13, 2022

Without a filename, the client and server have no common way to identify it, no way to tell it's type (MT uses file extensions), and no way for it actually get used anywhere.

This PR simply adds the ability to send ephemeral media from a file already in memory as a Lua string. Rather than having to save it to disk, have the engine read it, and then delete it from disk again.

@GoodClover
Copy link
Contributor Author

Any features of replacing already sent media are unrelated & waaaay outside the scope of this PR. They would go quite nicely hand-in-hand with this however.

@GoodClover GoodClover marked this pull request as ready for review September 13, 2022 01:36
@SmallJoker SmallJoker added Rebase needed The PR needs to be rebased by its author. and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Oct 23, 2022
@rubenwardy rubenwardy added the Concept approved Approved by a core dev: PRs welcomed! label Oct 23, 2022
@GoodClover
Copy link
Contributor Author

I would appreciate someone looking over the changes to lua_api.txt, as the explanation for dynamic media has got a bit long… which might be a sign the API changes themselves aren't so good.

@sfan5 sfan5 removed the Rebase needed The PR needs to be rebased by its author. label Nov 7, 2022
std::string to_player;
bool ephemeral = false;

if (lua_istable(L, 1)) {
getstringfield(L, 1, "filepath", filepath);
fromfile = getstringfield(L, 1, "filepath", filepath);
Copy link
Contributor

Choose a reason for hiding this comment

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

filepath should be checked for emptiness like in the other branch.

bool ok = true;
std::string filedata_file;
std::string &filedata = [&]() -> std::string & {
if (filedata_to && !filedata_to->empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if someone uses data = ""? Won't that cause addMediaFile to try to read from filepath? IMO you should add a separate parameter to signal that filedata_to is an input, rather than checking whether it is empty. data = "" should be supported out of principle.

@@ -3557,7 +3566,7 @@ bool Server::dynamicAddMedia(std::string filepath,
// Newer clients get asked to fetch the file (asynchronous)
pkt << token;
// Older clients have an awful hack that just throws the data at them
legacy_pkt.putLongString(filedata);
legacy_pkt.putLongString(*filedata);
Copy link
Contributor

Choose a reason for hiding this comment

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

filedata is allowed to be nullptr in the header declaration, but here it seems it is assumed not to be nullptr.

@GoodClover GoodClover closed this by deleting the head repository Mar 20, 2023
@sfan5
Copy link
Member

sfan5 commented Mar 20, 2023

Do you plan to continue working on this?

@GoodClover
Copy link
Contributor Author

GoodClover commented Mar 20, 2023

I am not knowledgeable of C++ enough to work on it. I just happened to bash something semi-working together.

@sfan5 sfan5 added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adoption needed The pull request needs someone to adopt it. Adoption welcomed! Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add dynamic media without using disk
8 participants