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

CSE Machine: Added animation for spread instruction #3114

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

alsonleej
Copy link
Contributor

@alsonleej alsonleej commented Mar 12, 2025

Description

Partially deals with #3110, further work can be done on array access out of bounds

Type of change

  • New feature (non-breaking change which adds functionality)
20250312-1921-34.2597022.mp4

Checklist

  • I have tested this code
  • I have updated the documentation

@coveralls
Copy link

coveralls commented Mar 13, 2025

Pull Request Test Coverage Report for Build 14290737373

Details

  • 0 of 49 (0.0%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 31.004%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/features/cseMachine/animationComponents/ArrayAccessAnimation.tsx 0 6 0.0%
src/features/cseMachine/CseMachineAnimation.tsx 0 11 0.0%
src/features/cseMachine/animationComponents/ArraySpreadAnimation.tsx 0 32 0.0%
Files with Coverage Reduction New Missed Lines %
src/features/cseMachine/animationComponents/ArrayAccessAnimation.tsx 1 0.0%
Totals Coverage Status
Change from base Build 14256181806: -0.1%
Covered Lines: 4892
Relevant Lines: 14883

💛 - Coveralls

@NhatMinh0208
Copy link
Contributor

NhatMinh0208 commented Mar 14, 2025

Looking at it, might take a while though since I'm busy

Copy link
Member

@s-kybound s-kybound left a comment

Choose a reason for hiding this comment

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

Looks good, but please remove the print statements.

@martin-henz martin-henz changed the title Added animation for spread instruction CSE Machine: Added animation for spread instruction Apr 1, 2025
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

Would be good if you have a screen recording showing the new animation in action!

craco.config.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert the formatting changes to this file (and just this file) too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay done, but now for some reason the lint coveralls failed Error: Bad response: 405 . Not sure why this is happening

Copy link
Member

Choose a reason for hiding this comment

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

Probably their service is down, just wait a while

@@ -28,6 +28,7 @@ export class ArrayAccessAnimation extends Animatable {
private resultAnimation: AnimatedTextbox;
private resultArrowAnimation?: AnimatedGenericArrow<StashItemComponent, Visible>;
private arrayUnit: ArrayUnit;
private out: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

What does out mean? Can clarify? From the name, it's not immediately clear what this does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to outOfRange. if the array access is our of range, outOfRange is false, true otherwise

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.

6 participants