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 TICKSLEFT binding #2890

Merged
merged 4 commits into from
Mar 9, 2021
Merged

Add TICKSLEFT binding #2890

merged 4 commits into from
Mar 9, 2021

Conversation

thexa4
Copy link
Contributor

@thexa4 thexa4 commented Mar 9, 2021

Can work as a solution to #2889, #145

Can work as a solution to KSP-KOS#2889, KSP-KOS#145
@thexa4 thexa4 marked this pull request as draft March 9, 2021 19:03
@Dunbaratu
Copy link
Member

I don't think it does everything desired by those issues, but it is a fine idea to have this.
I think the documenation should make it clear whether it's "instructions that were left prior to this call" or "instructions that are left afer this call".

Reading when shared.Cpu.instructionsPerUpdate gets incremented, it appears to get incremented after executing an instruction, so when the current instruction is evaluating the suffix and putting the answer on the stack, it's seeing the value minus one.

I think we can just move the increment line from CPU.cs to just before, rather than just after, the ExecuteInstruction and it won't mess anything up. It would make the result of this easier to nail down.

i.e. currently it does this on line 1505 of CPU.cs:

                    executeNext = ExecuteInstruction(currentContext, doProfiling);
                    ++InstructionsThisUpdate;

and it could be changed to this without introducing a problem, I think:

                    ++InstructionsThisUpdate;
                    executeNext = ExecuteInstruction(currentContext, doProfiling);

This would mean that semantically "InstructionsThisUpdate" is now counting how many instructions have happened including the current one, when before it was counting how many instructions have happened excluding the current one.

@thexa4 thexa4 marked this pull request as ready for review March 9, 2021 19:54
@Dunbaratu Dunbaratu merged commit 4b5ff82 into KSP-KOS:develop Mar 9, 2021
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