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

Add Progress #24

Merged
merged 8 commits into from
Jan 20, 2021
Merged

Add Progress #24

merged 8 commits into from
Jan 20, 2021

Conversation

onepiecefreak3
Copy link
Contributor

This adds the progress feature based on the IProgress interface. It's not much but enough. Maybe it can be done more memory efficient?

@onepiecefreak3 onepiecefreak3 mentioned this pull request Jan 18, 2021
@Nominom
Copy link
Owner

Nominom commented Jan 18, 2021

For the memory efficiency, just making the ProgressElement into a struct would make it allocate on the stack and remove some GC pressure. Also only reporting every 100 blocks or so (and last block) would make it take less processing and still maintain good progress estimate (if UI needs to be refreshed on every call to Report).

Should this have tests? Like launch a thread that monitors the progress and assert that the progress is always greater than or equal to the previous value. Then assert that progress is 100% once the operation is done.

@Nominom
Copy link
Owner

Nominom commented Jan 18, 2021

Another thing I was thinking of is, wouldn't it now show progress for only the current mipmap level? Might need to calculate the totalblocks from all mipmaps first, then keep a cumulative count that stays between mipmap level encodes.

…ecoder;

Add progress wrapping for multiple mipmap decoding;
@onepiecefreak3
Copy link
Contributor Author

This adds a wrapper for the IProgress to the OperationContext that can be aware of its current mipmap progress.
I also did some more refactoring regarding usage of the 3 different format enums. Namely I implemented using only CompressionFormat where possible by converting GlInternalFormat and DxgiFormat properly.
While doing that I found out how to solve issue #25 since dwFlags contains a flag regarding this behaviour it seems. I will fix that with this PR as well.

I don't know if the progress behaviour is testable. Any ideas how that can be done?

@Nominom
Copy link
Owner

Nominom commented Jan 19, 2021

The test can launch an async encoding method, and run a while loop afterwards. In the while loop the progress is checked that it is always equal to or greater than the previous progress values. After the operation is done, assert that progressed blocks is same as total blocks.

Also, the code looks good to me now. 👍

EDIT: ah you just pushed, I'll look again.

@onepiecefreak3
Copy link
Contributor Author

I think this feature can be merged now.

@Nominom
Copy link
Owner

Nominom commented Jan 20, 2021

Yeah, it looks good to me.

@Nominom Nominom merged commit ccf7dc8 into Nominom:master Jan 20, 2021
@onepiecefreak3 onepiecefreak3 deleted the progress branch January 20, 2021 19:29
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.

None yet

3 participants