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

Decorations: Generalise 'spawn by' to be used by all decoration types #4520

Closed
wants to merge 1 commit into from
Closed

Decorations: Generalise 'spawn by' to be used by all decoration types #4520

wants to merge 1 commit into from

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Sep 11, 2016

In lua_api.txt, make clear that 'place on' and 'spawn by' can be lists.
/////////////////////////////////////////////////

Requested in #4463
Tested.

To keep this simple just the placement point of the schematic is considered for 'spawn by', as this is for use with the flags 'place centre x' 'place centre z' and will usually be used for schematic trees which have a central trunk.

@paramat paramat added WIP The PR is still being worked on by its author and not ready yet. Action / change needed Code still needs changes (PR) / more information requested (Issues) and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) WIP The PR is still being worked on by its author and not ready yet. labels Sep 11, 2016
return true;

int nneighs = 0;
v3s16 dirs[16] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should make this static const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

@paramat
Copy link
Contributor Author

paramat commented Sep 12, 2016

Updated with requests.
Retested.

@@ -338,16 +338,63 @@ DecoSchematic::DecoSchematic()
}


size_t DecoSchematic::generate(MMVManip *vm, PcgRandom *pr, v3s16 p)
bool DecoSchematic::canPlaceSchematicDecoration(MMVManip *vm, v3s16 p)
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I missing something or is this function identical to canPlaceSimpleDecoration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first few lines are different, but those can be moved into 'generate' to allow de-duplicating this code, good idea will try this.

@kwolekr
Copy link
Contributor

kwolekr commented Sep 14, 2016

The commit message is a bit misleading.
Instead of saying that you allow schematic decorations to use spawn by, why not say that you generalized spawn by to all deco types? If L-system decorations ever get added, or any future decoration types for that matter, it too will have the same spawn by feature.

@paramat
Copy link
Contributor Author

paramat commented Sep 14, 2016

Seems correct to me but will update commit message.

In lua_api.txt, make clear that 'place on' and 'spawn by' can be lists.
@paramat paramat changed the title Decorations: Allow schematic decorations to use 'spawn by' Decorations: Generalise 'spawn by' to be used by all decoration types Sep 14, 2016
@paramat
Copy link
Contributor Author

paramat commented Sep 14, 2016

Updated with all requests. Retested.
Note the diff for lua_api.txt includes some lines i didn't change for some reason, seems good if you 'view' the file though.
Also: In lua_api.txt, make clear that 'place on' and 'spawn by' can be lists.

@kwolekr
Copy link
Contributor

kwolekr commented Sep 14, 2016

👍

@paramat
Copy link
Contributor Author

paramat commented Sep 14, 2016

b885950

@paramat paramat closed this Sep 14, 2016
@paramat paramat deleted the schemspawnby branch September 15, 2016 04:57
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.

None yet

3 participants