Port to python3 and other fixes #20
Conversation
|
Update- While testing my changes in 7ad9a6c , I get this error. Just noticed that reviewing changes for Physics Activity for Port to Python3 might help. I know PyClass_Type is a Python 2 type but I'm not sure if the error as relates to PR #19 ?
|
JuiP
changed the title
There was a problem hiding this comment.
Reviewed, I would suggest testing your changes once with #19 . I guess this could help in this error -
ImportError: elements/box2d_64/_Box2D.so: undefined symbol: PyClass_Type
|
Agreed. Now that #19 is merged, please rebase on new master. You may also need to install Box2D for Python 3. |
|
While rebasing there were multiple conflicts and my local branch diverged from origin/Fixes so I preferred to cherry-pick commits to a new branch origin/Python3.... Closing this PR but still working on the issue in branch Python3 |
|
I was expecting something to go wrong as 99ae5b0 was included here when it shouldn't have been. |
|
While rebasing, I checked out origin/master and then mistakes I made while git rebase:
|
|
Firstly, you didn't have to |
|
Sorry I just read your comment, will definitely like to know how to get this fixed !! It's kind of irritating to just desert the issues without finding a work-around! I will ping you when you are online tomorrow :) |
|
The social problem with a new pull request is that the relevant discussion on the changes is split across multiple browser pages and different notification threads in mail. I agree with @chimosky, and will put it slightly differently; there's never a need to close a pull request in order to handle a git conflict. It is best to fix the conflicts properly. At worst you can rewrite your commits in a new branch, then For your interest, a rebase at the time I suggested it (7ad9a6c) can go like this; There were no conflicts this way. |
|
As I have shifted this pr anyway, I'm going to try all sort of things to get this branch fixed too. In the process I may stumble upon new things I didn't know about git..... Just googling my way through this now, I will surely ask more specific questions if I am stuck at some point. Thankyou so much for helping! :) |
|
There are some other hacks you can practice to recover from things you don't yet understand;
Beware of googling; some advice predates new features or defaults of git that make it easier. |
|
I got my branch Fixes fixed with For reference these might be useful to make my process more clear: After HEAD@42, After googling, what I found was that my remote branch Fixes was different from my local branch Fixes which resulted in the above error when I tried to push. After rebasing, the reapplied commits have different hash therefore are considered as different commits. So at this step the a git push origin Fixes -f was expected but instead I did the error of git pull origin Fixes, now this resulted in fetching the HEAD of my remote branch Fixes. This fetched the remote Fixes which wasn't rebased, as a result there were conflicts as the changes in local Fixes were different from remote Fixes. I really hope I have properly understood what went wrong, could you confirm if what I stated is actually the case? @quozl |
|
Ok. Please do continue to experiment, as it is good learning. I've not been in this situation with respect to not being able to reopen a pull request, but you may be able to reopen by using Regarding the messages when you tried to rebase; that was caused by your earlier merge of the remote Fixes branch into your local Fixes branch, and;
Thanks for the |
|
Finallyyyy!! This mess is cleaned and fixed perfectly(Perfectly.... I hope so @chimosky @quozl). The way I got this done was-
I found a similar issue reported by some user: https://www.bountysource.com/issues/8777850-allow-to-reopen-pull-requests-after-a-force-push
I think their point is that Github tracks a reflog of every HEAD change while the PR is open (for... some reason?), but doesn't do this tracking when the pull request is closed, so they can't construct a history between what the branch's HEAD was when the PR was last open and what the HEAD of the PR's branch is now. I find that weird because when we push to the remote branch they still have the track to where the HEAD of the PR branch is! Nevermind there seems to be an issue with how they chose to disable the |
|
Yes everything is good now, thanks for sharing how you fixed it.
Yes you're right, Github tracks every commit that's been made until the PR was closed that's why you had to force push 2c20dcd first to get your remote up to the last commit before you closed the PR hence making reopening the PR possible, the force push of 670c1e2 could also work as it used to be 7ad9a6c before your rebase and they now had a common parent 99ae5b0 which is HEAD at master. |
|
Good to see my speculation was right. I see the branch has been rebased. Thanks. I get a ModuleNotFoundError in lib/myelements. Do you? |
|
Well yes! lib/myelements is not ported to python3 in this branch after rebasing which was done in Python3 branch..... I will apply the changes here! :) |
|
Thanks. I did then cherry-pick 44a23d8 and test.
This could be related to the call to This could be related to how physics.run calls pygame.init, which therefore displaces the initialisation done carefully by the PygameCanvas. In short, it looks like your port to Sugargame 1.3 has not followed the new API and test activity. |
|
@quozl made the required changes and tested, works for me.... Can you test again and merge? :) |
| @@ -593,7 +593,7 @@ def json_load(self, path, additional_vars={}): | |||
| jointDef.maxMotorTorque = joint['maxMotorTorque'] | |||
| self.world.CreateJoint(jointDef) | |||
|
|
|||
| for (k, v) in worldmodel['additional_vars'].items(): | |||
| for (k, v) in list(worldmodel['additional_vars'].items()): | |||
There was a problem hiding this comment.
This change can also be omitted
There was a problem hiding this comment.
I think, this change was made when you ran 2to3 to port lib/myelements and is a required change. .items() return a copy of the dictionary’s list of (key, value) pairs in python 2. However, in python 3 versions, items() returns iterators and never builds a list fully.
There was a problem hiding this comment.
As I have looked at Pippy and Physics activities, they have ported to Python3 before and we must follow those changes as they have been checked.
There was a problem hiding this comment.
I think, this change was made when you ran 2to3 to port lib/myelements
Yes,
and is a required change. .items() return a copy of the dictionary’s list of (key, value) pairs in python 2. However, in python 3 versions, items() returns iterators and never builds a list fully.
No, list() operation takes a lot of CPU than returning the iterator
Run this
from timeit import timeit
# timeit is a module to return the time taken by a line to complete
timeit("from itertools import permutations, product; permutations((product([1,2,3], [1,2,3])))")
timeit("from itertools import permutations, product; list(permutations(product([1,2,3], [1,2,3])))")@JuiP why do you think?
And what is the necessity of keeping the list() too? The for loop works without it too?
tools.py
Outdated
| @@ -344,7 +344,7 @@ def cancel(self): | |||
|
|
|||
| def getAllTools(): | |||
| this_mod = __import__(__name__) | |||
| all = [val for val in this_mod.__dict__.values() if isinstance(val, type)] | |||
| all = [val for val in list(this_mod.__dict__.values()) if isinstance(val, type)] | |||
There was a problem hiding this comment.
I guess the list() operator was not necessary. Moreover all is a built in function that changed from python2 to python3. It might be better to change all variable name to something different so that it does not shadow the builtin function
|
I tested it, works good, no errors, but something unusual happens for me. ( I can confirm this is not a part of my system configuration) As you might see, the circles which fell down of the screen in one instance, appears from the sky again on the next level. This might be because the fallen objects are not destroyed once its out of the screen boundary. Although it might not be a part of this PR, I just wanted to bring attention to it as a future issue, if its not a part of this port. (Please ignore the screen flickering, that is not the issue, the issue pointed out is the circles falling again and again) During the pause stage, If I add multiple very huge circles, and sometimes if the circles overlap with each other, this happens And, finally a fun thing I had devised of |
|
Wow! This is hilarious!!
I guess this was not taken care of in the code even before the port
|
|
Yes, exactly. This is one of those activities where intentional maintenance stopped. An activity's existence in our organisation doesn't mean it should necessarily be maintained. If you can improve it and fix it so it works, great. |
|
#20 (comment) check this out |
There was a problem hiding this comment.
Thanks @JuiP,
Now the Bridge Activity supports multiple resolutions, 8694d00 It also works on portrait mode too.
Few more important points to consider for the future:
-
Can erase the train
The train, apparently can be erased. This helps to gain negative cost (or profit), It should be against game ideology to erase the train the gain profit and win the game. -
Remove the hand button and circle.
Regarding the falling circles error I mentioned in the previous review, it would be better to remove the hand and circle tool, or let it be available on the detection of a particular environment variable.
import os
if os.getenv("DEBUG"):
# add Circle and Hand toolsor something similar.
- If the circle and the grab tool is removed, we can also get rid of the other PyBox2D assertion errors that happen.
- Port to python3 and other fixes #20 (comment)
Apart from that, this PR might be an improvement over themaster.
Though the developers intended to remove them, I think having them makes the activity a little more interesting! Have a look at this case, you can use the hand so that the train passes over the circle. What do you think @chimosky @quozl @srevinsaju ? If we decide to keep them in further releases, the issues which need to be fixed are:
|
I think it should be left as part of the game, it's something that a child will have to figure out themselves and that happens when they use the activity. That could also teach them about bugs. This is shown in the logs, although I can't tell how to reproduce as it happened just the first time I tested and didn't happen again. Also you could change the text that shows when |
|
Yes @chimosky, I was able to get the same Box2D assertion errors, and the Trains errors which caused, sometimes just wheels to come out as a result, and the rectangles of the train left out etc.. |
Well I tested again, tried all sorts of things. AssertionError occured when I tried pressing T multiple times this caused what @srevinsaju and @chimosky pointed out
I fixed this in 697c804. Pressing T will no longer connect it to the event creating a train until the train exits.
@srevinsaju you can no longer do this after 0a39b95 |
|
I don't plan on fixing these issues in this PR, maybe will raise an issue.
Maybe these observations may help: |
|
Thanks. Tested. Reviewed changes since my last review. Reviewed aggregate change. Is a substantial improvement; the activity works now, and can be used for the purpose. I agree with others there are some other improvements that can be made, but I think I'm ready to merge. If you've suggested an improvement, raise an issue. If you wish to work on an improvement, raise a pull request, not an issue. |
The activity was designed for specific pixel display, so everything was out of focus when I tested master on Sugar VM (800x600). These changes fixed it for me. I scaled the dimensions proportionately.
Please review the changes till d56b3dd @quozl @chimosky ! :)
I also took the opportunity to port the activity to python3, that is still in progress...