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

removed multiscript #8718

Merged
merged 1 commit into from
May 11, 2017
Merged

Conversation

karroffel
Copy link
Contributor

After some testing it is clear that this feature isn't as useful as most people thought.
The opposite effect happened. Scripts are designed to be attached to one object, so there are unintuitive behaviours.

Now nobody can say we didn't try again 😛

RIP MultiScript

Copy link
Contributor

@neikeq neikeq left a comment

Choose a reason for hiding this comment

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

I told you this was madness.

@akien-mga
Copy link
Member

ELOGTOOSHORT

Please be more explicit as to why you're removing it. The description of the commit log should mention that it was readded in commit $hash, but finally reverted as it brought various difficulties that would make multiscripts too convoluted. Please think of future contributors ;) If we had had those details in the previous commit removing the feature, we might not have retried it.

removes MultiScript which was re-added in godotengine#8502 (aka 4c14700).
This feature didn't turn out to be as useful as most expected. It causes more troubles than it does good.
@akien-mga
Copy link
Member

akien-mga commented May 11, 2017

Also almost everyone is using a capitalized commit log title, why must you be different? ;)

@karroffel karroffel force-pushed the remove-multiscript branch from b329ead to 15bce7f Compare May 11, 2017 19:25
@karroffel
Copy link
Contributor Author

@akien-mga I'm special 😛

Added a better git log

@akien-mga
Copy link
Member

I know I'm annoying, but it's very important in big projects that commit logs are very explicit. They should not only clearly define what they do ("removed multiscript", that's already clear enough, though "Remove MultiScript module" would look better :P), but also explain the why of a decision. If the decision is trivial, it's of course not necessary, but as soon as some explanations would be useful, they should be given (as they are in your PR description, but nobody will read GitHub PR descriptions 3 years in the future when we're using a self-hosted federated GitLab instance :P).

@punto-
Copy link
Contributor

punto- commented May 11, 2017 via email

@karroffel
Copy link
Contributor Author

@punto- See #8546

@akien-mga akien-mga merged commit a48b8bf into godotengine:master May 11, 2017
@karroffel karroffel deleted the remove-multiscript branch June 21, 2017 16:29
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.

4 participants