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

Fixed banana generation on catch specific maps #2611

Merged
merged 6 commits into from May 26, 2018
Merged

Conversation

@frankhjwx
Copy link
Contributor

@frankhjwx frankhjwx commented May 22, 2018

Fix the bug when generating bananas on catch specific maps because of lacking IHasXPosition property.

frankhjwx added 2 commits May 22, 2018
@frankhjwx frankhjwx closed this May 23, 2018
@frankhjwx frankhjwx reopened this May 23, 2018
@peppy
Copy link
Member

@peppy peppy commented May 23, 2018

Your PR description says IHasEndTime but the diff is adding IHasXPosition.

@frankhjwx
Copy link
Contributor Author

@frankhjwx frankhjwx commented May 23, 2018

Sorry it's my fault, catch objects should have IHasXPosition property for object generating.

@peppy
Copy link
Member

@peppy peppy commented May 23, 2018

i’m not sure this is the correct fix as spinners in catch don’t actually have an x position.

@peppy peppy added this to the May 2018 milestone May 23, 2018
@smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented May 23, 2018

A better fix would be to re-order CatchBeatmapConverter to generate spinners if there is no position data, rather than returning early.

@frankhjwx
Copy link
Contributor Author

@frankhjwx frankhjwx commented May 23, 2018

@smoogipoo That's right, the main problem is caused by if (positionData == null) judgement in CatchBeatmapConvertor

Still not very familiar with git operation, failing to discard my changes on ConvertSpinner.cs.

@peppy peppy modified the milestones: May 2018, June 2018 May 25, 2018
peppy added 2 commits May 26, 2018
@peppy peppy dismissed their stale review via 8b2a6b8 May 26, 2018
@peppy
peppy approved these changes May 26, 2018
@peppy peppy merged commit b0d2af1 into ppy:master May 26, 2018
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants