-
Notifications
You must be signed in to change notification settings - Fork 9
feat: slow calculation algo back to more adaptive impl #17
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
base: master
Are you sure you want to change the base?
Conversation
Also it would be better to revert this change too, since slot is now in right place |
Can you please explain what's broken exactly? I spent a bunch of time testing the functionality the last couple of days (for #15) and didn't find any obvious bugs.
I was not following GTNH development at the time, so all I can judge is the original PR. It looks like the functionality was wanted by the team, but you took a while to respond to the feedback (only a month later). I agree that it is frustrating to see your code get changed in a direction you don't agree with and I also agree that some of the changes could've been put in a followup PR. However, I don't think the changes were done malicicously and more because you didn't respond for a month. Also, remember that all a PR is, is a branch you want to have merged into another branch and you opened that branch in our shared repository to which everyone in the dev team has access to and the expectation should be that others might add changes to your branches (with the goal of improvement, of course, not griefing), GTNH is a collaborative effort IMO. If you're not comfortable with that, you can make PRs from a personal fork. That way others in the team don't have direct push access. That being said I would like @koolkrafter5 to explain why they needed to change the algorithm. There is no explanation in the commit message or a PR comment.
I don't understand what mean here. Which change should be reverted? You are linking to this PR here. |
@wlhlm The idea of this PR is to go back to more adaptive algorithm, which was in initial PR: P.S. I also changed message about PR to revert :) |
I see now. Thanks for clarification. I'd be interested to hear why @koolkrafter5 decided to change the algo. |
I changed the algorithm to both be a lot simpler than the original implementation and favor tall layouts over wide ones, since having the UI be too wide makes it so NEI is unusable while in the UI. I'm working on a different fix to find rectangular layouts which I will make in a different PR. This is because I don't like how there are two nested loops that waste a lot of time iterating even after finding a layout (the |
My PR #14 's feature was broken by further fixes, so lets make backpacks great again
P.S. How is it possible that the prev PR, which make backpack adaptive was broken by the same PR due to changes by other people? This is disrespectful