Skip to content

Add TICKSLEFT binding#2890

Merged
Dunbaratu merged 4 commits intoKSP-KOS:developfrom
thexa4:feature/ticksleft
Mar 9, 2021
Merged

Add TICKSLEFT binding#2890
Dunbaratu merged 4 commits intoKSP-KOS:developfrom
thexa4:feature/ticksleft

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