Skip to content

Refactored code#4

Merged
Cappycot merged 11 commits intoCappycot:masterfrom
Banjobeni:master
Aug 24, 2022
Merged

Refactored code#4
Cappycot merged 11 commits intoCappycot:masterfrom
Banjobeni:master

Conversation

@Banjobeni
Copy link
Contributor

Hi Cappycot

As discussed, this pull request includes all the refactorings I worked on during the last couple of weeks. Given that there is a significant chance that upcoming big releases of Derail Valley will break compatibility with mods, I think it will be vital to have the code base in good shape.

In addition, there are two new features:

  • Dispatch hint also shown while hovering over jobs.
  • A detailed (technical) description of the job task tree, with finished tasks written in green. Written as a debug tool to understand whats going on, but I think it's quite useful while playing as well. I am happy to discuss where we want to take this, and feel free to disable it for a release if you like.

I cannot test VR as I do not have any VR equipment. I think the refactored code base should work for VR as well but it would be great if you could verify this.

Please let me know what you think. I will be happy to get back to you with more details and features that I like to implement in the coming weeks.

Cheers
Banjobeni

@Banjobeni
Copy link
Contributor Author

Hovering over jobs displays dispatch hint:
20220821121507_1

An example of the (experimental) task tree summary:
20220821123613_1

@Cappycot
Copy link
Owner

Cappycot commented Aug 21, 2022

Starting my look. (Still wrapping things up with other mods so I'll be a bit slow on the draw still.)

A few notes to start:

  • I noticed you are using a JetBrains IDE. Not sure if Visual Studio will play well with the annotations when I pull the code up for a closer look.
  • For desktop users, I like the idea of pasting all steps, but we'd probably need to modify or stretch the GUI textures to prevent text from overflowing.
    • (Perhaps showing the first unfinished step and pointing to the cars for that?)
  • We'd probably want to not say the track name for non-yard tracks since they just have random ids like "#S704#T". I remember leaving that in there since I never got to it.
    • (We do already have a pointer to the consist anyway.)
  • As for VR systems, I'll need to rebuild and take a look. (I'm on a Valve Index.)

Again, thanks for the code cleanup! I first wrote this mod when I was just starting out on C#/Unity modding and even today my software dev skills aren't close to good, so this refactored system should work out better than what I had planned.

I'm also reachable via Altfuture Discord if you're in there.

@Banjobeni
Copy link
Contributor Author

  • I noticed you are using a JetBrains IDE. Not sure if Visual Studio will play well with the annotations

I use Visual Studio with the Jetbrains ReSharper plugin. The annotations are actually defined in one of the Unity assemblies and I decided to use them since they're already there. From a Visual Studio perspective, those are just attributes with no special meaning, so they shouldn't get in your way.

  • For desktop users, I like the idea of pasting all steps, but we'd probably need to modify or stretch the GUI textures to prevent text from overflowing.

Fully agree. What I really want is to simplify the task tree using a visitor pattern and use that to generate nice flowing texts that tell you what to do and maybe hint at what's to do next. Like "[cars location info]. They need to be taken to C3L for loading and then moved to G1S."

  • We'd probably want to not say the track name for non-yard tracks

Never gave that a thought - but that should actually be a low-hanging fruit that I might pick next. We might want to differentiate between tracks that are within a station (saying "on a track in MF") and section tracks. Might be a little bit tricky to determine, though.

@DerJJ
Copy link

DerJJ commented Aug 22, 2022

I just tested this on a Valve Index, for me the hint messages never disappear. Once you take a sheet in hand, that message will stay. If you drop it and take another one, both will stay.

I compiled this myself with VS2022.

@Banjobeni
Copy link
Contributor Author

Hi DerJJ. Thanks a lot for testing! I will have a look at it and try to find and fix it (blindly).

@Banjobeni
Copy link
Contributor Author

Banjobeni commented Aug 22, 2022

@DerJJ, i pushed a quick fix for an obvious bug - floatie object never being destroyed. It would be great if you could have another look at it.

@Cappycot
Copy link
Owner

I finished skimming over most of the refactor - this is much cleaner than I could have refactored it. (I slept through my software engineering course.) I plan on merging this within the week once we confirm the VR floaties go away as expected. (Thanks, DerJJ!)

I probably want to bring attention to a few things I remembered with the original dispatcher for future notes/interest.

First, we ideally shouldn't use a timer to force dispatcher hint updates every second. Using Harmony postfixes for event-driven updates is better, but that involves hours of reverse-engineering in dn/ILspy. Heck, the floatie loading process should be event-driven, but I guess I never found the best place to do it.

Second, there was never a guarantee that Job objects only have a single sequential Task that contains the rest of the tasks in nested form. Depending on what the sim update brings, the implementation of GetFirstUnfinishedTasks(Task) may need to be changed. (I would hope not since Tasks themselves are marked as sequential, parallel, or an actual work item.)

Third, part of my original plan before I burnt out was to have the hint change depending on the page of the job book you were on.
e.g. If you are on a page that tells you to load cargo, the hint should tell you either where the cars are for that step or if it's done already.

If you have Discord, I can be found in the Altfuture Discord under the same username as my GitHub. It's faster to reach me there if necessary.

@DerJJ
Copy link

DerJJ commented Aug 23, 2022

I will check it tonight (~12 hours, UTC +1), sorry I can't manage to test this earlier.

@DerJJ
Copy link

DerJJ commented Aug 23, 2022

An additional note while reviewing your code:

From your initial comment I understood that this is a cleanup to be prepared for the next patch. One new major feature of Simulator will be i18n. However, I noticed you hardcoded all the user output. Any plans to extract that and read the (user output) strings from a resource?

@Cappycot
Copy link
Owner

We can probably get i18n working after this merge.

@DerJJ
Copy link

DerJJ commented Aug 23, 2022

Can confirm that it works fine in VR now.

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.

3 participants

Comments