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

expose endTime and endTick #521

Conversation

jacopomaroli
Copy link

Issues

Fixes #520

Proposed changes

expose endTime and endTick

Checklist

  • I consent that this change becomes part of alphaTab under it's current or any future open source license
  • Changes are implemented
  • Existing builds tests pass
  • New tests were added

Further details

  • This is a breaking change
  • This change will require update of the documentation/website

@jacopomaroli
Copy link
Author

sorry, I could not run tests. I got a huge amount of

[1] 15 01 2021 17:44:18.575:INFO [http.server]: save visual error undefined
[1]     ✗ zeros-on-dive-whammys-off
[1] 	Difference between original and new image is too big: 472/8065 (5.85%) (undefined)
[1] 	Error
[1] 	    at <Jasmine>
[1] 	    at Function.<anonymous> (dist/lib.test/test/index.js:47860:43)
[1] 	    at <Jasmine>
[1] 	    at fulfilled (dist/lib.test/test/index.js:47653:62)

which seems to be unrelated so I stopped the process.

I did

nvm use lts/fermium
npm i
npm run test

@Danielku15
Copy link
Member

Danielku15 commented Jan 15, 2021

Unfortunately this change will not deliver the desired results. I recommend you have a look first on how other values are passed back and forth within the synthesizer. You have to keep in mind that the actual synthesizer (the AlphaSynth instance) is living within the Web Worker and the api.player is just a component communicating with the worker.

With this change player.endTick will be just undefined. The IAlphaSynth interface synchronizes the API on the api.player and the actual implementation in AlphaSynth.

Regarding the tests: the visual tests are currently optimized for running on Chrome in Windows. It's just a guess but if you're on Linux/MacOS there might be too big differences in the rendering. Unfortunately there is no way to enforce consistent text rendering across browsers.

@Danielku15
Copy link
Member

@jacopomaroli I assume with my latest changes and discussion on #520 we can close this PR without merging?

@jacopomaroli
Copy link
Author

yes, please procede closing this PR. Thanks!

@Danielku15 Danielku15 closed this Jan 17, 2021
@jacopomaroli jacopomaroli deleted the feature/expose_sequencer_data branch January 17, 2021 18:50
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