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

[STCC-121] - Fix conversion of PointToBrick #60

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

Hyderow
Copy link
Contributor

@Hyderow Hyderow commented Feb 16, 2018

This seems to fix the conversion of the PointToBricks. The program from the ticket still crashes though.

@@ -226,7 +226,7 @@ class _ScratchToCatrobat(object):
"turnLeft:": catbricks.TurnLeftBrick,
"heading:": catbricks.PointInDirectionBrick,
"forward:": catbricks.MoveNStepsBrick,
"pointTowards:": catbricks.PointToBrick,
"pointTowards:": lambda sprite_name: catbricks.PointToBrick(SpriteFactory().newInstance(SpriteFactory.SPRITE_SINGLE, sprite_name)),
Copy link
Contributor

@cfkh cfkh Feb 16, 2018

Choose a reason for hiding this comment

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

You would now create a new sprite instance every time there is a PointToBrick.
But the sprite given to the constructor of PointToBrick could already exist at this point.
You'll probably need a register handler. Look in datacontainer if sprite already exists, then use this sprite instance. Otherwise you would need to create a new sprite instance. Somewhere there is a list called upcoming_sprites. It'll need to be added to this list, I think.

@Hyderow Hyderow changed the base branch from master to develop February 17, 2018 17:26
@Hyderow
Copy link
Contributor Author

Hyderow commented Feb 17, 2018

It should be implemented properly now. Should we think about adding a workaround for pointing towards the mouse cursor?

@cfkh
Copy link
Contributor

cfkh commented Feb 17, 2018

What you did seems good to me.

It would be nice if you could include a workaround for Point To mouse.
In Pocket Code the equivalent to the mouse position would obviously be the touch position.
Be aware that in Pocket Code the Point To brick doesn't accept a formula as an argument just Sprites.

Idea (but you might have a better one)
Therefore you will need to use the Point In Direction brick. The workaround will need a bit of math too, because you will need to calculate an "angle between points" (which is obviously not existing).
You could therefore calculate the length of the hypothetical line connecting these two points. Giving you a hypotenuse. You could also calculate the length of the adjacent leg, by simply calculating the absolute x difference. Now you can finally calculate your degrees by acos(adjacent leg/hypotenuse).

Note
Please also include tests for your implementation in the test_converter module.

@Hyderow
Copy link
Contributor Author

Hyderow commented Feb 20, 2018

I decided to add a dummy mouse sprite that always updates its position to the last touch position as a workaround. If we do it this way we don't have to insert several blocks into the script, but just have a single new sprite.

@cfkh
Copy link
Contributor

cfkh commented Feb 20, 2018

Sounds good to me.

@cfkh
Copy link
Contributor

cfkh commented Feb 20, 2018

  1. Please make a program in Pocket Code which has two sprites in them and let one point at the other at start.
    Does this work? On my phone nothing happens.

  2. When starting a program containing PointTo mouse Pocket Code will crash. When again choosing the mouse sprite as an argument in Pocket Code the program doesn't crash.

  3. Please rebase on develop

@Hyderow
Copy link
Contributor Author

Hyderow commented Feb 20, 2018

1: This seems to work fine for me. I tried it with 2 sprites that only have a when started script with a single PointTo brick. The result was that both sprites point towards each other.

2: This crash is really weird. It happens even without the mouse workaround being in the program and seems to resolve itself eventually (just by randomly opening scripts touching anything in them). I'll look into it a bit more.

@cfkh
Copy link
Contributor

cfkh commented Feb 20, 2018

Okay. Weird. I guess it's a Pocket Code problem. Please update us on your findings per comment right here and/or in discord.

@Hyderow
Copy link
Contributor Author

Hyderow commented Feb 21, 2018

After some more testing I managed to reproduce this bug on the master branch. The program I got it with is simply 2 sprites with a when started script and a gotoSprite, pointing at the other sprite(see attached file).

If you look at the converted code of the program, one of the sprites is not in the spriteslist as a sprite, but the entire definition of it is in the destinationSprite field of the goTo brick of the other sprite.
I think the cause of the bug may have to do with the way we add a new sprite to the upcoming sprites map in the register handler.

goto-crash.zip

Update: I think this might also be the cause for STCC-132. It crashes at the same line in both Angry birds! and my example program.

@cfkh
Copy link
Contributor

cfkh commented Feb 21, 2018

I have also tested a few programs and can confirm this behaviour.

We don't deal with upcoming sprites any different than with other sprites, we only store the reference for later.
Currently we use some Catroid functionality which does the serialization for us:

def program_source_for(catrobat_program): storage_handler = catio.StorageHandler() code_xml_content = storage_handler.XML_HEADER code_xml_content += storage_handler.getXMLStringOfAProject(catrobat_program)

Could this be a serialization problem on Catroid's side?
If this is the case: What we (possibly) could do to avoid this, is that we reorder the sprites in the scene's spritelist, so that no sprite is used before being referenced. (Of course if we have for instance two sprites referencing each other we can't do anything.)

Please look deeper into this. This is pretty important as it not only affects one bricks functionality, but probably a few. Take the time you need. You can of course also grab other issues in the meantime.

* Add register handler for PointToBrick
* Add workaround for mouse cursor
* Fix GoTo Brick spinner values (Fixes STCC-132)
@Hyderow
Copy link
Contributor Author

Hyderow commented Feb 22, 2018

Turns out we didn't correctly set the spinner value in the registerhandler for the goTo Block. Changing these values to the correct ones fixed the crashes.

@cfkh
Copy link
Contributor

cfkh commented Feb 22, 2018

Please try converting a selfmade Scratch program where the first sprite (Sprite 1) in the scene doesn't have any scripts and the second one (Sprite 2) has a WhenGreenFlag script with only a PointTowards block referring to Sprite1. Deploy the converted project on your phone's Pocket Code folder. Crash at starting the converted program?

@Hyderow
Copy link
Contributor Author

Hyderow commented Feb 22, 2018

Doesn't crash for me.

@cfkh
Copy link
Contributor

cfkh commented Feb 23, 2018

OK

@cfkh cfkh merged commit 2558011 into Catrobat:develop Feb 23, 2018
cfkh added a commit that referenced this pull request Mar 10, 2018
* [STCC-123] - Fix key sprites not being added correctly (#59)

* Only add sprites to the scene once.
* Correctly add all keys to listened_keys
* Fix workaround for space key

* STCC-128 Deleted Uservariables are now created by the converter (#62)

* STCC-129 Conversion now allows floats as hight&width errormessage fixed (#64)

* STCC-122 blocks setting missing looks are now replaced by notebricks (#65)

* [STCC-121] - Fix conversion of PointToBrick (#60)

* Add register handler for PointToBrick
* Add workaround for mouse cursor
* Fix GoTo Brick spinner values (Fixes STCC-132)

* STCC-120: Update Class Hierarchy Excerpt (#67)

*  TESTFIX: Change spinnerSelection values in tests  (#68)

* STCC-120: Update Class Hierarchy Excerpt

* TESTFIX: Change spinnerSelection values in tests
cfkh pushed a commit that referenced this pull request Mar 10, 2018
* Add register handler for PointToBrick
* Add workaround for mouse cursor
* Fix GoTo Brick spinner values (Fixes STCC-132)
@AntiDog AntiDog mentioned this pull request Feb 25, 2019
AntiDog added a commit that referenced this pull request Feb 25, 2019
Scratch 3.0 Integration

Since Scratch 2.0 the converter is no longer working. With these changes the converter should be working again.

Changes:

Scratch 3.0 Integration, Bug fixes, Media converter fixes:

[STCC-123] - Fix key sprites not being added correctly (#59)
STCC-128 Deleted Uservariables are now created by the converter (#62)
STCC-129 Conversion now allows floats as hight&width errormessage fixes (#64)
STCC-122 blocks setting missing looks are now replaced by notebricks (#65)
[STCC-121] - Fix conversion of PointToBrick (#60)
STCC-120: Update Class Hierarchy Excerpt (#67)
TESTFIX: Change spinnerSelection values in tests (#68)
STCC-141 Broadcast message can be a number which causes an exception
Merge pull request #73 from AntiDog/STCC-141
STCC-144 Fixed only keys of one sprite being added. (#75)
STCC-136 PNG files now get scaled (#74)
STCC-143 Unknown fonts get replaced, fonts now get scaled (#76)
STCC-145 Support mouse cursor for distanceTo block (#77)
PNGs get now moved by rotation center, some remove of dead code, SVG Texts now onscreen (#80)
new class hierachy excerpt and empty images no longer crash catroid (1px gets set) (#81)
STCC-151 Implement parser for Scratch3 files (#82)
STCC-148 Rework pseudo mouse (#79)
STCC-114 Any key is now recognized in keypressed blocks and scripts (#78)
STCC-146 Execute keyPressed scripts repeatedly if key is held down (#83)
STCC-164 Basic unittests for Scratch3 look blocks (#84)
STCC-174 Insert correct default values for user blocks (#87)
STCC-170 Refactor methods in Scratch3 parser to be more readable (#85)
Use alternative scratch3 sprite attribute to read look filenames (#89)
STCC-173 Add more logging to Scratch3 parser (#86)
STCC-172 tspans in svg cause errors (#88)
dbg (#91) - (Debug.py for debugging purposes)
[STCC-169] basic sound block testcases (#90)
[STCC-168] Add unit tests for sensor blocks (#92)
[STCC-162] Basic testcases for data blocks (#93)
[STCC-163] Add tests for control blocks (#94)
[STCC-165] Add motion block testcases (#95)
[STCC-166] Add unit tests for operator blocks (#96)
[STCC-178] - Integrate Scratch3 into converter (#97)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants