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

Place schematic (on vmanip): Enable use of 'place center' flags #7072

Closed
wants to merge 1 commit into from
Closed

Place schematic (on vmanip): Enable use of 'place center' flags #7072

wants to merge 1 commit into from

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Feb 23, 2018

Place schematic (on vmanip): Enable use of 'place center' flags

For 'place schematic' and 'place schematic on vmanip' APIs.
Fix 'place center' code to properly centre schematics.
Fix some comments.
///////////////

For #7066
@Gael-de-Sailly please could you test this?

@paramat paramat added @ Mapgen WIP Feature ✨ PRs that add or enhance a feature Low priority labels Feb 23, 2018
@gaelysam
Copy link
Contributor

I've tested your changes with a quickly written test mod that generates one pine tree per mapgen chunk.

local vmanip = true
local schem = minetest.get_modpath("default") .. "/schematics/pine_tree.mts"
local flags = "place_center_x,place_center_z"

minetest.register_on_generated(function(minp, maxp, seed)
	local pos = {x=math.floor((minp.x+maxp.x)/2), y=math.floor((minp.y+maxp.y)/2), z=math.floor((minp.z+maxp.z)/2)}
	if vmanip then
		local vm = minetest.get_mapgen_object("voxelmanip")
		minetest.place_schematic_on_vmanip(vm, pos, schem, "random", nil, false, flags)
		vm:write_to_map()
	else
		minetest.place_schematic(pos, schem, "random", nil, false, flags)
	end
	minetest.set_node(pos, {name="default:stone"})
end)

I expected the tree to be placed at (7,7,7), on the stone block, but it's placed at (6,7,6). As far as I know the pine tree schem doesn't have extra air rows that makes the trunk not being at the center, so it's misplaced.
screenshot_20180223_181450
By the way I did the same changes for minetest.place_schematic, it behaves exactly the same.

@gaelysam
Copy link
Contributor

I just found the cause of the issue and fixed it in my branch.

@paramat
Copy link
Contributor Author

paramat commented Feb 24, 2018

Correct, see https://github.com/minetest/minetest/blob/master/src/mapgen/mg_decoration.cpp#L363

	if (flags & DECO_PLACE_CENTER_X)
		p.X -= (schematic->size.X - 1) / 2;
	if (flags & DECO_PLACE_CENTER_Z)
		p.Z -= (schematic->size.Z - 1) / 2;

I will do this for 'place schematic' too.

@paramat paramat changed the title Place schematic on vmanip: Enable use of 'place center' flags Place schematic (on vmanip): Enable use of 'place center' flags Feb 24, 2018
@paramat
Copy link
Contributor Author

paramat commented Feb 24, 2018

Updated.
@Gael-de-Sailly since i'm busy could you test this updated PR, testing 'place schematic' too? Also please test nothing breaks when there is no 'flags' field used (for backwards compatibility).
Once tested i can probably merge this as trivial.

@gaelysam
Copy link
Contributor

All of this works. Both functions work as expected, and there is no bug when flags is omitted. I think you can merge.

@paramat
Copy link
Contributor Author

paramat commented Feb 24, 2018

Thanks.

@paramat paramat removed the WIP label Feb 24, 2018
For 'place schematic' and 'place schematic on vmanip' APIs.
Fix 'place center' code to properly centre schematics.
Fix some comments.
@paramat paramat added the Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines label Feb 27, 2018
@paramat
Copy link
Contributor Author

paramat commented Feb 27, 2018

c610643

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature @ Mapgen Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants