ST-Apps / PoGo-UWP

UWP Client for Pokemon Go
832 stars 341 forks source link

ViewModel ObservableCollections are not updated continuously #678

Closed robertmclaws closed 8 years ago

robertmclaws commented 8 years ago

Because the GameClient ObservableCollections are only being loaded into the ViewModels when they are navigated to, I don't believe they are staying in sync. I need to investigate a more direct synchronization approach, and its effect on performance.

One avenue would be to use an IoC container to create a single GameClient instance for the app, that then gets injected into each ViewModel in the constructor, and then the pages can bind directly ViewModel.GameClient.SomeCollection, unless some other type of filtering is needed.

Another might be to wire in OnCollectionChanged handlers on each ViewModel. Will investigate.

ST-Apps commented 8 years ago

You have too keep in mind that those collections are updated only when needed, to reduce network usage. For example, if there's no user action (maybe he's just navigating the map or browsing settings) there's no need to update the inventory.

EDIT: removed OT messages to keep the discussion clean.

robertmclaws commented 8 years ago

@ST-Apps You're right, but I'm not referring to that. For example: Right now there is a bug where the amount of Pokémon candy on a Pokémon's detail page does not change once you transfer another instance of the same Pokémon type. If I have 3 Ekans and 49 Ekans candy, and transfer 1, I'll still have 49 Ekans candy when I open one of the other two.

I haven't had a chance to look yet, but I THINK it's because the ViewModel inventory property does not have a two-way binding to the GameClient. It only happens once when the Inventory page is first loaded.

skyne commented 8 years ago

I'm thinking on this and some other related problems for a while, and more i'm thinking, i'm more convinced, this whole structure needs a refactor. I'm not completely sure on the final form, but there's something i can imagine:

The whole game logic and comm handling needs to be separeted from views and viewmodels, move, the response handlers, interaction methods etc. to GameClient or Client classes (according to what they do) and keep the VM's clean, these should contain only codes responsible for displaying things. In this way, we win in more aspect:

Edit:

I know, my thinking sometimes could be really hard to understand and thats combined with my english knowledge could be a hard mess :D So just to be more clear: I can imagine an interface on a GameClient class. Through this interface, the whole game could be playable, whatever UI it has. If i want to play through a ConsoleApplication, it can be done etc.

Please keep in mind, this is just a suggestion, and needs much more brainstorming/planning. I just thought on this for a while and wanted to share ideas :)

jakubsuchybio commented 8 years ago

Well to be honest. In fixing the https://github.com/ST-Apps/PoGo-UWP/issues/645 I had to access GameMapPageViewModel to UpdatePlayerData. So I agree, that every API handling should be refactored outside of views. Now it starts to look like a spaghetti a bit.

robertmclaws commented 8 years ago

Well, architectures like MvvmLight have ways for ViewModels to message each other internally. It's pretty common, and not a bad pattern at all.

Ideally, the game mechanics should be unit testable without a) talking to Niantic's servers, and b) without requiring UI.

Not sure that requires a giant refactor right now though. My first step will be to see if subscribing to collections solves the problem for now.

ST-Apps commented 8 years ago

@advancedrei Uh, I just forgot to update Player's currencies after tranfering, thanks for noticing it!

@jakubsuchybio actually the methods are inside GameClient (you have UpdateProfile and UpdatePlayerStats) so I'm not quit sure of what you mean.

By the way, here's a quick explanation of why we have this situation: the first release had a Singleton ViewModel that was used by all the pages, so that data was really shared between them with minimal effort. This cause a lot of navigation issues, because the navigation handlers were called in unexpected moments and it was quite painful to understand what to do after finishing the navigation.

This made me move a big part of the logic to that static GameClient, so that we can keep shared data but without the previous navigation issues, as each page has his own VM now.

Now, of course this can be improved, but I didn't find a better solution yet so I'm open to suggestions.

ST-Apps commented 8 years ago

Let's continue the discussion on PoGo-Devs/PoGo#1