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

Add hedit #268

Merged
merged 5 commits into from
Mar 2, 2021
Merged

Add hedit #268

merged 5 commits into from
Mar 2, 2021

Conversation

jlillis
Copy link
Contributor

@jlillis jlillis commented Jan 4, 2021

hedit is an in-game vehicle handling editor. It is widely used to play with vehicle handling in development and freeroam environments. The code quality is on par with existing resources in this repo, with a number of the original authors also contributing to this repo regularly.

This commit adds hedit by performing a quick and dirty subtree merge. It is located under [editor].

git-subtree-dir: [editor]/hedit
git-subtree-split: 0f158262e3d7ec9a6dc4548f5f9e419327ef72fe
@xLive
Copy link
Member

xLive commented Jan 4, 2021

I think [gameplay] is more suitable for it

@jlillis
Copy link
Contributor Author

jlillis commented Jan 27, 2021

Based on feedback I've moved hedit to [gameplay]. If there aren't any objections, I'll be merging this in the next few days.

@jlillis
Copy link
Contributor Author

jlillis commented Feb 2, 2021

Original documentation stated that this resource needs admin privileges to work properly, but I can't find any actual reason for that in my testing and review of the code.

Also note that the original hedit repository will be archived after this is merged.

@jlillis
Copy link
Contributor Author

jlillis commented Feb 16, 2021

Could another repo member give this an approving review? Repo policy prevents me from merging this without one.

@Dutchman101
Copy link
Member

Dutchman101 commented Feb 16, 2021

Could another repo member give this an approving review? Repo policy prevents me from merging this without one.

I don't think hedit follows coding guidelines everywhere, and it's also not very optimized (just watch ipb for its memory usage and other metrics) but I personally think we should make an exception for hedit due to its gameplay value, even if we don't expect someone wants to work on those points first.

It's also more bloated than it needs to be for its functionality, having older bloated resources in official resources is 1 thing, but nowadays we aim for a change to good practises when it comes to adding new resources and code (would go against adding other resources, that happen to already be bloated).

Because i would like another opinion on whether or not we should make an exception, i will leave the approval review to another member (MTA team) to see if they agree. If no other opinion or objections arrive soon, then ill approve it anyways.

@jlillis
Copy link
Contributor Author

jlillis commented Feb 16, 2021

I don't think hedit follows coding guidelines everywhere, and it's also not very optimized (just watch ipb for its memory usage and other metrics) but I personally think we should make an exception for hedit due to its gameplay value, even if we don't expect someone wants to work on those points first.

I'm not aware of any official coding guidelines for mtasa-resources. I have been trying to make some progress with them on #250 but it has been slow. If there is anything that jumps out at you I can probably resolve some of it, but I just don't see much work to do here. Code quality and performance seems to be "good enough". There is some bloat that could be cut (the translations stuff and logging come to mind) but it didn't seem urgent to me.

@Dutchman101
Copy link
Member

Dutchman101 commented Feb 16, 2021

There is some bloat that could be cut (the translations stuff and logging come to mind)

In my eyes, the bloat is everywhere.. rewriting the entire resource from scratch (it can be much tidier to do the same thing) seems like the only fix. But that is clearly not going to happen, and I feel letting hedit through is reasonable.

I'm not aware of any official coding guidelines for mtasa-resources

My bad.. it was loosely applied as the requirements you will meet during code review. Good to see the work on formal guidelines though

Copy link
Member

@Dutchman101 Dutchman101 left a comment

Choose a reason for hiding this comment

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

If no other opinion or objections arrive soon, then ill approve it anyways

It's time

@jlillis jlillis merged commit dafde27 into multitheftauto:master Mar 2, 2021
@jlillis jlillis deleted the add-hedit branch June 7, 2021 21:46
@Deltanic
Copy link

Deltanic commented Oct 2, 2021

It would have been appreciated if the original author was at least informed of this decision. It kind of goes against community principles to blatantly steal an entire repository, even if a license is missing. I'd have happily added new maintainers if someone would like to improve my work. Sad.

@patrikjuvonen
Copy link
Contributor

It would have been appreciated if the original author was at least informed of this decision. It kind of goes against community principles to blatantly steal an entire repository, even if a license is missing. I'd have happily added new maintainers if someone would like to improve my work. Sad.

In regards to this post, we apologized and resolved this issue on Discord earlier today with @Deltanic. It turns out this communication issue was a result of a misunderstanding between the hedit project developers.

The MTA team and this repository's maintainers are ultimately responsible for double checking attribution requirements and license compatibility etc. before a merge can take place. We absolutely do not tolerate any stealing or assumptions.

We found that our review process lacks two important parts:

  1. Require a code license with code owners described to avoid misunderstandings and giving others the ability to confirm licensing and ownership through the source.

  2. To avoid biased merges, require a maintainer unrelated to the pull request to review it and go through the licensing and to check if something stands out.

We promise to do better with vetting pull requests in the future to make sure everyone is satisfied.

We asked whether @Deltanic would like us to revert the merge but he said it is not necessary.

Sorry for any inconvenience from behalf of the team and thank you for reaching out to us to resolve the situation.

@ricksterhd123
Copy link
Contributor

It's done now right?
/close

@patrikjuvonen patrikjuvonen added this to the 1.5.9 milestone Apr 22, 2022
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.

6 participants