Skip to content

Conversation

@sfan5
Copy link
Collaborator

@sfan5 sfan5 commented Apr 20, 2025

context on last commit:
Currently a texture like default_stone.png^[applyfiltersformesh is generated once, uploaded to the GPU and the image data thrown away.
When we want to introduce array textures we may need the same texture image multiple times and/or generated in advance. This change makes that possible.

To do

This PR is Ready for Review.

How to test

  1. play game
  2. nothing should have changed

@sfan5 sfan5 added @ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature labels Apr 20, 2025
@grorp
Copy link
Member

grorp commented May 2, 2025

To test the texture re-using, I made a this patch (obviously broken in different ways, just for fun):

Diff
diff --git a/src/client/client.cpp b/src/client/client.cpp
index 1859356b9..03b3ecd4e 100644
--- a/src/client/client.cpp
+++ b/src/client/client.cpp
@@ -1900,71 +1900,9 @@ float Client::getCurRate()
 
 void Client::makeScreenshot()
 {
-	irr::video::IVideoDriver *driver = m_rendering_engine->get_video_driver();
-	irr::video::IImage* const raw_image = driver->createScreenShot();
-
-	if (!raw_image)
-		return;
-
-	const struct tm tm = mt_localtime();
-
-	char timetstamp_c[64];
-	strftime(timetstamp_c, sizeof(timetstamp_c), "%Y%m%d_%H%M%S", &tm);
-
-	std::string screenshot_dir;
-
-	if (fs::IsPathAbsolute(g_settings->get("screenshot_path")))
-		screenshot_dir = g_settings->get("screenshot_path");
-	else
-		screenshot_dir = porting::path_user + DIR_DELIM + g_settings->get("screenshot_path");
-
-	std::string filename_base = screenshot_dir
-			+ DIR_DELIM
-			+ std::string("screenshot_")
-			+ std::string(timetstamp_c);
-	std::string filename_ext = "." + g_settings->get("screenshot_format");
-	std::string filename;
-
-	// Create the directory if it doesn't already exist.
-	// Otherwise, saving the screenshot would fail.
-	fs::CreateDir(screenshot_dir);
-
-	u32 quality = (u32)g_settings->getS32("screenshot_quality");
-	quality = MYMIN(MYMAX(quality, 0), 100) / 100.0 * 255;
-
-	// Try to find a unique filename
-	unsigned serial = 0;
-
-	while (serial < SCREENSHOT_MAX_SERIAL_TRIES) {
-		filename = filename_base + (serial > 0 ? ("_" + itos(serial)) : "") + filename_ext;
-		if (!fs::PathExists(filename))
-			break;	// File did not apparently exist, we'll go with it
-		serial++;
-	}
-
-	if (serial == SCREENSHOT_MAX_SERIAL_TRIES) {
-		infostream << "Could not find suitable filename for screenshot" << std::endl;
-	} else {
-		irr::video::IImage* const image =
-				driver->createImage(video::ECF_R8G8B8, raw_image->getDimension());
-
-		if (image) {
-			raw_image->copyTo(image);
-
-			std::ostringstream sstr;
-			if (driver->writeImageToFile(image, filename.c_str(), quality)) {
-				sstr << "Saved screenshot to '" << filename << "'";
-			} else {
-				sstr << "Failed to save screenshot '" << filename << "'";
-			}
-			pushToChatQueue(new ChatMessage(CHATMESSAGE_TYPE_SYSTEM,
-					utf8_to_wide(sstr.str())));
-			infostream << sstr.str() << std::endl;
-			image->drop();
-		}
-	}
-
-	raw_image->drop();
+	errorstream << "Reloading textures" << std::endl;
+	clearTextureNameCache();
+	m_tsrc->rebuildImagesAndTextures();
 }
 
 void Client::pushToEventQueue(ClientEvent *event)
diff --git a/src/client/imagesource.cpp b/src/client/imagesource.cpp
index e39dc2955..2a3652814 100644
--- a/src/client/imagesource.cpp
+++ b/src/client/imagesource.cpp
@@ -20,13 +20,19 @@
 // SourceImageCache Functions //
 ////////////////////////////////
 
-SourceImageCache::~SourceImageCache() {
+void SourceImageCache::clear()
+{
 	for (auto &m_image : m_images) {
 		m_image.second->drop();
 	}
 	m_images.clear();
 }
 
+SourceImageCache::~SourceImageCache()
+{
+	clear();
+}
+
 void SourceImageCache::insert(const std::string &name, video::IImage *img, bool prefer_local)
 {
 	assert(img); // Pre-condition
@@ -83,7 +89,7 @@ video::IImage* SourceImageCache::getOrLoad(const std::string &name)
 				<< name << "\"" << std::endl;
 		return nullptr;
 	}
-	infostream << "SourceImageCache::getOrLoad(): Loading path \"" << path
+	errorstream << "SourceImageCache::getOrLoad(): Loading path \"" << path
 			<< "\"" << std::endl;
 	video::IImage *img = driver->createImageFromFile(path.c_str());
 
diff --git a/src/client/imagesource.h b/src/client/imagesource.h
index b5e3d8d3a..ddcb81610 100644
--- a/src/client/imagesource.h
+++ b/src/client/imagesource.h
@@ -22,6 +22,7 @@ class SourceImageCache {
 	~SourceImageCache();
 
 	void insert(const std::string &name, video::IImage *img, bool prefer_local);
+	void clear();
 
 	video::IImage* get(const std::string &name);
 
@@ -65,6 +66,7 @@ struct ImageSource {
 	bool m_setting_bilinear_filter;
 	bool m_setting_anisotropic_filter;
 
+public:
 	// Cache of source images
 	SourceImageCache m_sourcecache;
 };
diff --git a/src/client/texturesource.cpp b/src/client/texturesource.cpp
index cf5752ac3..d9df1d1bf 100644
--- a/src/client/texturesource.cpp
+++ b/src/client/texturesource.cpp
@@ -486,6 +486,7 @@ void TextureSource::insertSourceImage(const std::string &name, video::IImage *im
 void TextureSource::rebuildImagesAndTextures()
 {
 	MutexAutoLock lock(m_textureinfo_cache_mutex);
+	m_imagesource.m_sourcecache.clear();
 
 	/*
 	 * Note: While it may become useful in the future, it's not clear what the
@@ -525,6 +526,7 @@ void TextureSource::rebuildTexture(video::IVideoDriver *driver, TextureInfo &ti)
 	if (!img) {
 		// new texture becomes null
 	} else if (t_old && t_old->getColorFormat() == img->getColorFormat() && t_old->getSize() == img->getDimension()) {
+		errorstream << "replacing " << ti.name << " in place" << std::endl;
 		// can replace texture in-place
 		std::swap(t, t_old);
 		void *ptr = t->lock(video::ETLM_WRITE_ONLY);
diff --git a/src/script/lua_api/l_settings.cpp b/src/script/lua_api/l_settings.cpp
index 089b8a01e..416541219 100644
--- a/src/script/lua_api/l_settings.cpp
+++ b/src/script/lua_api/l_settings.cpp
@@ -47,7 +47,7 @@ static inline int checkSettingSecurity(lua_State* L, const std::string &name)
 	}
 
 	const char *disallowed[] = {
-		"main_menu_script", "shader_path", "texture_path", "screenshot_path",
+		"main_menu_script", "shader_path", "screenshot_path",
 		"serverlist_file", "serverlist_url", "map-dir", "contentdb_url",
 	};
 	for (const char *name2 : disallowed) {
switch.mp4

Works :)

@grorp
Copy link
Member

grorp commented May 3, 2025

Experimented again, this is amazing :)
If the ITexture re-using could be relied on, that would open up so many possibilities so easily...

Replacing the stone texture via dynamic_add_media, even with dependent textures being updated properly:

dynamic_add_media_replacement.mp4
Diff
diff --git a/src/client/imagesource.cpp b/src/client/imagesource.cpp
index e39dc2955..2ab46ac70 100644
--- a/src/client/imagesource.cpp
+++ b/src/client/imagesource.cpp
@@ -83,7 +83,7 @@ video::IImage* SourceImageCache::getOrLoad(const std::string &name)
 				<< name << "\"" << std::endl;
 		return nullptr;
 	}
-	infostream << "SourceImageCache::getOrLoad(): Loading path \"" << path
+	errorstream << "SourceImageCache::getOrLoad(): Loading path \"" << path
 			<< "\"" << std::endl;
 	video::IImage *img = driver->createImageFromFile(path.c_str());
 
diff --git a/src/client/texturesource.cpp b/src/client/texturesource.cpp
index cf5752ac3..cd92a9e22 100644
--- a/src/client/texturesource.cpp
+++ b/src/client/texturesource.cpp
@@ -525,6 +525,7 @@ void TextureSource::rebuildTexture(video::IVideoDriver *driver, TextureInfo &ti)
 	if (!img) {
 		// new texture becomes null
 	} else if (t_old && t_old->getColorFormat() == img->getColorFormat() && t_old->getSize() == img->getDimension()) {
+		errorstream << "replacing " << ti.name << " in place" << std::endl;
 		// can replace texture in-place
 		std::swap(t, t_old);
 		void *ptr = t->lock(video::ETLM_WRITE_ONLY);
diff --git a/src/server.cpp b/src/server.cpp
index 008b91022..000336ba8 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -3684,17 +3684,6 @@ bool Server::dynamicAddMedia(const DynamicMediaArgs &a)
 			<< filepath << std::endl;
 	}
 
-	// Do some checks
-	auto it = m_media.find(filename);
-	if (it != m_media.end()) {
-		// Allow the same path to be "added" again in certain conditions
-		if (a.ephemeral || it->second.path != filepath) {
-			errorstream << "Server::dynamicAddMedia(): file \"" << filename
-				<< "\" already exists in media cache" << std::endl;
-			return false;
-		}
-	}
-
 	if (!m_env && (!a.to_player.empty() || a.ephemeral)) {
 		errorstream << "Server::dynamicAddMedia(): "
 			"adding ephemeral or player-specific media at startup is nonsense"

@sfan5
Copy link
Collaborator Author

sfan5 commented May 3, 2025

In principle there is no reason an ITexture instance couldn't totally change (different dimensions or opengl texture) while staying at the same memory location.
In practice this is likely to cause problems and I think we will ultimately need a way to explicitly refresh textures in all relevant parts.

@sfan5 sfan5 merged commit 486fb7c into luanti-org:master May 3, 2025
17 checks passed
@sfan5 sfan5 deleted the 123456790 branch May 3, 2025 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants