widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing

I think this is a cool feature, thanks for working on this.

I am a bit unhappy about the postload() design approach, but that is not the fault of this branch. We are constructing partially initialized objects right and left and that gets pretty confusing - when is which data valid? I think it would be better to collecting all inter-object dependencies while loading the objects into a collection that is passed around to the loading functions. Later, they are all resolved at one point and missing objects can be reported then. What do you think? Do you want to try this in this branch or defer?

Diff comments:

> === modified file 'data/tribes/init.lua'
> --- data/tribes/init.lua	2017-02-12 09:10:57 +0000
> +++ data/tribes/init.lua	2017-09-03 11:19:20 +0000
> @@ -11,12 +11,33 @@
>  --
>  -- Basic load order (first wares, then immovables etc.) is important,
>  -- because checks will be made in C++.
> --- Also, enhanced/upgraded units need to come before their basic units.
>  --
>  
>  tribes = wl.Tribes()
>  include "scripting/mapobjects.lua"
>  
> +-- Load all init.lua files in the given table of directory names
> +function load_directories(directories)
> +   -- Helper function to check for file name endings
> +   function string.ends(haystack, needle)
> +      return needle == '' or string.sub(haystack, -string.len(needle)) == needle
> +   end
> +
> +   while #directories > 0 do
> +      local filepath = directories[1]
> +      table.remove(directories, 1)

directories:remove(1) does not work?

> +      if path.is_directory(filepath) then

same here: filepath:is_directory() ?

> +         for idx, listed_path in ipairs(path.list_directory(filepath)) do
> +            if path.is_directory(listed_path) then
> +               table.insert(directories, listed_path)
> +            elseif string.ends(listed_path , "init.lua") then
> +               include(listed_path)
> +            end
> +         end
> +      end
> +   end
> +end
> +
>  print("┏━ Running Lua for tribes:")
>  
>  print_loading_message("┗━ took", function()
> 
> === modified file 'src/logic/map_objects/tribes/tribe_descr.h'
> --- src/logic/map_objects/tribes/tribe_descr.h	2017-07-19 20:40:32 +0000
> +++ src/logic/map_objects/tribes/tribe_descr.h	2017-09-03 11:19:20 +0000
> @@ -157,6 +157,9 @@
>  		return ship_names_;
>  	}
>  
> +	void add_building(const std::string& buildingname);
> +	void update_trainingsites_proportions(const std::string& new_site = "");

passing an empty string is semantically a pretty different function. Pull apart into two different functions? The only shared code is the if (used_percent < 100) and it doesn't hurt to duplicate that check in each function.

> +
>  private:
>  	// Helper function for adding a special worker type (carriers etc.)
>  	DescriptionIndex add_special_worker(const std::string& workername);
> 
> === modified file 'src/logic/map_objects/tribes/tribes.cc'
> --- src/logic/map_objects/tribes/tribes.cc	2017-03-23 07:36:36 +0000
> +++ src/logic/map_objects/tribes/tribes.cc	2017-09-03 11:19:20 +0000
> @@ -170,6 +170,20 @@
>  	}
>  }
>  
> +void Tribes::add_custom_building(const LuaTable& table) {
> +	const std::string tribename = table.get_string("tribename");
> +	if (Widelands::tribe_exists(tribename)) {
> +		TribeDescr* descr = tribes_->get_mutable(tribe_index(tribename));
> +		const std::string buildingname = table.get_string("buildingname");
> +		descr->add_building(buildingname);
> +		if (descr->get_building_descr(descr->building_index(buildingname))->type() == MapObjectType::TRAININGSITE) {

Can the if() not be in add_building?

> +			descr->update_trainingsites_proportions(buildingname);
> +		}
> +	} else {
> +		throw GameDataError("The tribe '%s'' has no preload file.", tribename.c_str());
> +	}
> +}
> +
>  size_t Tribes::nrbuildings() const {
>  	return buildings_->size();
>  }
> @@ -329,7 +346,61 @@
>  	}
>  }
>  
> +void Tribes::add_worker_buildcost(const WorkerBuildcost& buildcost) {
> +	postload_workers_buildcost_.push_back(buildcost);
> +}
> +
> +void Tribes::add_mapobject_enhancement(const MapObjectEnhancement& becomes) {
> +	postload_mapobject_enhancements_.push_back(becomes);
> +}
> +
>  void Tribes::postload() {
> +	// Some workers have other workers as build cost, so we postload those in order to perform the necessary checks.
> +	for (const WorkerBuildcost& buildcost : postload_workers_buildcost_) {
> +		if (!worker_exists(buildcost.worker)) {
> +			throw GameDataError(
> +			   "Trying to add a worker buildcost to non-existing worker '%s'", buildcost.worker.c_str());
> +		}
> +		if (!worker_exists(buildcost.needed_worker)) {
> +			throw GameDataError(
> +			   "The worker '%s' to be added as builcost to the worker '%s' does not exist", buildcost.needed_worker.c_str(), buildcost.worker.c_str());
> +		}
> +		workers_->get_mutable(safe_worker_index(buildcost.worker))->add_worker_to_buildcost(buildcost.needed_worker, buildcost.quantity);
> +	}
> +	postload_workers_buildcost_.clear();

Suggestion for clarity: group all members only needed for postloading in a private struct Postload data and keep a unique_ptr to it which you can then just reset at the end of this function. This makes it clear while reading that only postloading requires this data.

> +
> +	// Likewise, more experienced workers and advanced buildings might not have been available yet when a lower-level worker or building was loaded.
> +	for (const MapObjectEnhancement& enhancement : postload_mapobject_enhancements_) {
> +		switch (enhancement.type) {
> +		case MapObjectType::BUILDING:
> +			if (!building_exists(enhancement.name)) {
> +				throw GameDataError(
> +				   "Trying to add a building enhancement to non-existing building '%s'", enhancement.name.c_str());
> +			}
> +			if (!building_exists(enhancement.enhanced_name)) {
> +				throw GameDataError(
> +				   "The building '%s' to be added as enhancement to the building '%s' does not exist", enhancement.enhanced_name.c_str(), enhancement.name.c_str());
> +			}
> +			buildings_->get_mutable(safe_building_index(enhancement.name))->set_enhances_to(enhancement.enhanced_name);
> +			break;
> +		case MapObjectType::WORKER:
> +			if (!worker_exists(enhancement.name)) {
> +				throw GameDataError(
> +				   "Trying to add a worker enhancement to non-existing worker '%s'", enhancement.name.c_str());
> +			}
> +			if (!worker_exists(enhancement.enhanced_name)) {
> +				throw GameDataError(
> +				   "The worker '%s' to be added as enhancement to the worker '%s' does not exist", enhancement.enhanced_name.c_str(), enhancement.name.c_str());
> +			}
> +			workers_->get_mutable(safe_worker_index(enhancement.name))->set_becomes(enhancement.enhanced_name);
> +			break;
> +		default:
> +			NEVER_HERE();
> +		}
> +	}
> +	postload_mapobject_enhancements_.clear();
> +
> +	// Add building info to wares here, since wares were loaded first.
>  	for (DescriptionIndex i = 0; i < buildings_->size(); ++i) {
>  		BuildingDescr& building_descr = *buildings_->get_mutable(i);
>  
> 
> === modified file 'src/logic/map_objects/tribes/tribes.h'
> --- src/logic/map_objects/tribes/tribes.h	2017-03-23 07:36:36 +0000
> +++ src/logic/map_objects/tribes/tribes.h	2017-09-03 11:19:20 +0000
> @@ -148,6 +151,22 @@
>  	/// Complete the Description objects' information with data from other Description objects.
>  	void postload();
>  
> +	/// Some workers have other workers as part of their buildcost. If the other worker hasn't been loaded yet, it will need to be added during postload.
> +	struct WorkerBuildcost {
> +		const std::string worker;

do we not have a WareIndex for this worker already when constructing this?

> +		const std::string needed_worker;
> +		const Quantity quantity;
> +	};
> +	void add_worker_buildcost(const WorkerBuildcost& buildcost);
> +
> +	/// Enhanced buildings/workers might not have been loaded yet when a more basic type is being loaded, so we will need to add some of them during postload.
> +	struct MapObjectEnhancement {
> +		const MapObjectType type; // Worker or building
> +		const std::string name;

same here.

> +		const std::string enhanced_name;
> +	};
> +	void add_mapobject_enhancement(const MapObjectEnhancement& becomes);

suggestion: make this more generic. maybe: add_unresolved_mapobject_load_dependency.

> +
>  private:
>  	std::unique_ptr<DescriptionMaintainer<BuildingDescr>> buildings_;
>  	std::unique_ptr<DescriptionMaintainer<ImmovableDescr>> immovables_;
> 
> === modified file 'src/logic/map_objects/tribes/worker_descr.cc'
> --- src/logic/map_objects/tribes/worker_descr.cc	2017-04-21 17:03:26 +0000
> +++ src/logic/map_objects/tribes/worker_descr.cc	2017-09-03 11:19:20 +0000
> @@ -60,17 +60,18 @@
>  					throw GameDataError(
>  					   "a buildcost item of this ware type has already been defined: %s", key.c_str());
>  				}
> -				if (!tribes.ware_exists(tribes.ware_index(key)) &&
> -				    !tribes.worker_exists(tribes.worker_index(key))) {
> -					throw GameDataError("\"%s\" has not been defined as a ware/worker type (wrong "
> -					                    "declaration order?)",
> -					                    key.c_str());
> -				}
> -				int32_t value = items_table->get_int(key);
> -				uint8_t const count = value;
> -				if (count != value)
> +				int32_t temp_quantity = items_table->get_int(key);
> +				const uint8_t quantity = temp_quantity;
> +				if (quantity != temp_quantity) {
>  					throw GameDataError("count is out of range 1 .. 255");
> -				buildcost_.insert(std::pair<std::string, uint8_t>(key, count));
> +				}
> +				if (tribes.ware_exists(tribes.ware_index(key)) ||

Why not do it for everything in postload? We have the logic split throughout two places otherwise. Same above for building dependencies.

> +					 tribes.worker_exists(tribes.worker_index(key))) {
> +					buildcost_.insert(std::make_pair(key, quantity));
> +				} else {
> +					// The buildcost's worker wasn't loaded yet, so we'll try this again in postload.
> +					egbase->mutable_tribes()->add_worker_buildcost({name(), key, quantity});
> +				}
>  			} catch (const WException& e) {
>  				throw GameDataError("[buildcost] \"%s\": %s", key.c_str(), e.what());
>  			}
> @@ -91,8 +92,14 @@
>  
>  	// Read what the worker can become and the needed experience
>  	if (table.has_key("becomes")) {
> -		becomes_ = egbase_.tribes().safe_worker_index(table.get_string("becomes"));
>  		needed_experience_ = table.get_int("experience");
> +		const std::string becomes_name = table.get_string("becomes");

do everything in post.

> +		if (egbase_.tribes().worker_exists(becomes_name)) {
> +			set_becomes(becomes_name);
> +		} else {
> +			// The expert worker wasn't loaded yet, so we'll try this again in postload.
> +			egbase->mutable_tribes()->add_mapobject_enhancement({MapObjectType::WORKER, name(), becomes_name});
> +		}
>  	}
>  
>  	// Read programs
> 
> === modified file 'src/map_io/map_scripting_packet.cc'
> --- src/map_io/map_scripting_packet.cc	2017-01-25 18:55:59 +0000
> +++ src/map_io/map_scripting_packet.cc	2017-09-03 11:19:20 +0000
> @@ -67,18 +67,29 @@
>  }
>  
>  void MapScriptingPacket::write(FileSystem& fs, EditorGameBase& egbase, MapObjectSaver& mos) {
> -	fs.ensure_directory_exists("scripting");
> -
> +
> +	// Write all .lua files that exist in the given 'path' in 'map_fs' to the 'target_fs'.
> +	auto write_dir = [] (FileSystem& target_fs, FileSystem* map_fs, const std::string& path) {

this does not capture anything from its envirconment. Make it a standalone function in the anonymous namepsace.

> +		if (map_fs) {
> +			target_fs.ensure_directory_exists(path);
> +			for (const std::string& script :
> +			     filter(map_fs->list_directory(path),
> +			            [](const std::string& fn) { return boost::ends_with(fn, ".lua"); })) {
> +				size_t length;
> +				void* input_data = map_fs->load(script, length);
> +				target_fs.write(script, input_data, length);
> +				free(input_data);
> +			}
> +		}
> +	};
> +
> +	// Write any scenario scripting files in the map's basic scripting dir
>  	FileSystem* map_fs = egbase.map().filesystem();
> -	if (map_fs) {
> -		for (const std::string& script :
> -		     filter(map_fs->list_directory("scripting"),
> -		            [](const std::string& fn) { return boost::ends_with(fn, ".lua"); })) {
> -			size_t length;
> -			void* input_data = map_fs->load(script, length);
> -			fs.write(script, input_data, length);
> -			free(input_data);
> -		}
> +	write_dir(fs, map_fs, "scripting");
> +
> +	// Write any custom scenario tribe entities
> +	if (map_fs->file_exists("scripting/tribes/init.lua")) {
> +		write_dir(fs, map_fs, "scripting/tribes");
>  	}
>  
>  	// Dump the global environment if this is a game and not in the editor


-- 
https://code.launchpad.net/~widelands-dev/widelands/dynamic_tribe_loading/+merge/329198
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/dynamic_tribe_loading.