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 Process Listing and Attach by PID to Autosplitting API #721

Merged
merged 6 commits into from
Oct 1, 2023

Conversation

PARTYMANX
Copy link
Contributor

@PARTYMANX PARTYMANX commented Sep 9, 2023

Adds two functions to the API:

process_attach_pid attaches to a process by referencing its PID.
process_list lists all PIDs associated with processes with the name passed in.

Together, these allow an autosplitter to define its own logic for discovering and attaching to processes.

Sometimes start time or largest PID don't quite get the right process. For example, Tony Hawk's Pro Skater 1+2 has a bootstrap and game process, both with the same name. They start in the same second, so the regular process_attach logic often selects the incorrect process. This pull request allows an author to check each process closer to determine which one is desired.

@PARTYMANX PARTYMANX changed the title Add Process Listing and Attach by PID to API Add Process Listing and Attach by PID to Autosplitting API Sep 9, 2023
@CryZe CryZe self-requested a review September 9, 2023 10:23
@CryZe
Copy link
Collaborator

CryZe commented Sep 11, 2023

Some early thoughts:

  1. This guarantees PIDs always fit into u32. Is this something we can guarantee across the board? i.e. what if a future operating system uses u64s? Should we maybe just use u64 to be safe?
  2. I think here we probably want to fill the buffer, even if it is too small, i.e. truncate the resulting list. This is because the auto splitter may not have an allocator, and may know that if anything the process will only be open twice or so anyway, so it uses a fixed size array.
  3. I don't think a list of 0 PIDs should be an error. The user can interpret that.
  4. This (the previous two points) also means that the function can't actually fail. I'm not sure if we should still return a boolean (maybe in the future the underlying sysinfo crate will return errors when it fails to list the processes).

@PARTYMANX
Copy link
Contributor Author

  1. Good question. I'm looking into it a bit just out of curiosity and it seems like people are pretty set on 32-bit these days but who knows how that might change. At the very least, it looks like sysinfo is only ever casting from/to a u32 so I assume it's a safe bet.
  2. That's good thinking. Should we notify the caller that the list was truncated at all? I guess it should be fairly obvious if the return number is always modified.
  3. Agreed
  4. In this case, I think we're fine not returning a boolean. I can't come up with any sort of user-induced failure or case where actually having the information that a failure occurred is more useful. Chances are that the caller would just try again on the next tick.

I'll get to making modifications soon

@CryZe
Copy link
Collaborator

CryZe commented Sep 12, 2023

I've made some changes locally, let me push them first. I'll get to it after work.

@PARTYMANX
Copy link
Contributor Author

Hey, I wanted to double check the status of this. Have you made more progress on that work?

@CryZe
Copy link
Collaborator

CryZe commented Oct 1, 2023

Alright, I made all these changes. The PID is u64 now, just to be safe. And an error is still returned in case process listing would return an error (which also is just to be safe, because sysinfo doesn't actually return any errors here, but we may not always keep using that library).

@CryZe CryZe added enhancement An improvement for livesplit-core. feature A new user visible feature for livesplit-core. auto splitting This is about the auto splitting implementation. labels Oct 1, 2023
@CryZe CryZe added this to the v0.14 milestone Oct 1, 2023
@CryZe CryZe merged commit 7068dd1 into LiveSplit:master Oct 1, 2023
77 checks passed
CryZe added a commit to LiveSplit/asr that referenced this pull request Oct 1, 2023
Adds two functions:

`Process::attach_by_pid` attaches to a process by referencing its PID.
`Process::list_by_name` / `Process::list_by_name_into` lists all PIDs associated with processes with the name passed in.

Together, these allow an autosplitter to define its own logic for discovering and attaching to processes.  

Sometimes start time or largest PID don't quite get the right process.  For example, Tony Hawk's Pro Skater 1+2 has a bootstrap and game process, both with the same name.  They start in the same second, so the regular `Process::attach` logic often selects the incorrect process.  This pull request allows an author to check each process closer to determine which one is desired.  (rather messy) example of usage here: https://github.com/PARTYMANX/thps-autosplitter/blob/thps12-disambiguate/src/lib.rs#L19

Companion pull request to LiveSplit/livesplit-core#721.  Solves #50.

Co-authored-by: Christopher Serr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto splitting This is about the auto splitting implementation. enhancement An improvement for livesplit-core. feature A new user visible feature for livesplit-core.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants