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 server-specific resource loading for objects, room textures, music, sounds and rooms. #117

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

skittles1
Copy link
Contributor

Next iteration of #115.

Checks subfolders first for files (e.g. /rooms/105/, /sounds/105/) before loading a default from the parent directory (e.g. /rooms/, /sounds/) for each legacy resource file.

Preloading moved to OgreClient::HandleGetLoginMessage() but locks the client for a couple seconds after the Connect button is pressed. There's probably a better way to do this (check the threads at a later time, before displaying char selection?) but I'm not sure how to do this.

There are now 3 OpenMeridian servers open and due to differing update
schedules and content, the same .roo file may be different on each
server. Currently this is the case for Raza, where the version of the
city (7 rooms anyway) is different on 103/112 and 105.

To allow the single Ogre client to work on all 3 servers, I have changed
.roo loading to first check a server-specific folder (currently this is
the server name/number) for any .roo files before checking the parent
rooms directory. 103 would have its changed rooms in \rooms\103\, 105 in
\rooms\105\ etc.

In BaseClient.Connect(), the ServerSubFolder field of ResourceManager
is set to the selected server's name (103, 105 etc.). Server-specific
room files (and later, other legacy resources) are then added to the
dictionaries.

Preloading legacy resource occurs after this point, instead of on
client startup.
Added for music, sounds, objects and roomtextures.
@cyberjunk
Copy link
Owner

I'm basically OK with most of the parts in here.

However I'm a bit concerned about the preloading:
As far as I'm getting the change, it will be fully in background and it's not guaranteed it's done once the user selects a toon and logs in by pressing "Login".
Assuming someone has a slow system (dualcore+hdd) the preloading might not be done when login of the toon happens, either causing a bad delay on login (while already attackable) or in worst case causing multithreading issues due to trying to access a resource which is currently also preloaded.

@@ -844,6 +836,14 @@ namespace Meridian59 { namespace Ogre
{
ConnectionInfo^ info = Config->SelectedConnectionInfo;
SendLoginMessage(info->Username, info->Password);

// preload legacy resources
ResourceManager->Preload(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfect. It's exactly where I would have placed it.
It's when all the other server related stuff like strings or ignore-list is loaded.
I'm just concerned about the user-feedback on the preloading... and the fact it's not blocking a login until it's done.

If preloading any of the legacy resources is enabled, the loading bar
will be activated upon clicking connect, and will disappear when
preloading has been completed (user cannot login before this is
complete).
@skittles1
Copy link
Contributor Author

Added the loading bar, won't disappear until preloading is done (and then it is replaced by char select).

@cyberjunk
Copy link
Owner

Nice. I hoped it wouldn't require too many changes for the additional loading-bar,
but if ea96956 is all that's required that is really cool.

@skittles1
Copy link
Contributor Author

I was expecting to have to change a lot also, was surprised when it worked pretty much flawlessly the first time. Only other thing I had to do was add the check for config options so that a loading bar isn't displayed briefly if the user doesn't have preload enabled.

@cyberjunk
Copy link
Owner

I reviewed this a bit further, following points:

  1. It seems you didn't touch the Init() method in ResourceManager.cs. This method is called from the Init() in BaseClient.cs and it seems even with your pullrequest here it initially fills all the resource-dictionaries as before. However since all your new "AddServerObjects()" and others have a Clear() call in them, it probably wasn't noticed so far? I guess you can remove the parts in ResourceManager's Init() which fill the dictionaries initially as this needs to be done at a later point in time...

  2. I'm not perfectly happy with the calls to "File.Exists()" within the LoadRoom() and other methods. The inital idea of the code in ResourceManager's Init() was to register all existing files and afterwards avoid any I/O calls for file existance checks. So technically any File.Exists check (e.g. for determining) which file to load should be placed within your several new AddServerServerObject(), AddServerRoomTextures() and so on. The problem here is that there is no way to store these data so I see why you need all this code again in the specific GetRoom(), GetSound() methods to reconstruct the filepath of the actual file. This should definitely be handled somehow when the dictionaries are filled, the final GetRoom(), GetSound() and so on must not require this extensive path-determining and file.existance checks

@cyberjunk
Copy link
Owner

  1. Would you please add the following method to ResourceManager.cs (before or behind SelectStringDictionary()). This is simply your current code you placed directly into the Connect() of BaseClient.cs. Then in Connect() please call your new function right after SelectStringDictionary()

public void SelectServerResources(string ServerSubFolder)
{
this.ServerSubFolder = ServerSubFolder;
// Load server-specific resources if present
AddServerObjects();
AddServerRoomTextures();
AddServerRooms();
AddServerSounds();
AddServerMusic();
}

PS: Afterwards please make the AddXY() methods protected...

@skittles1
Copy link
Contributor Author

For point 1), I left Init() as-is in case any of the resources are used for creating the menu screen, since it happens before picking a server it'll use the resources in the parent directory (/rooms/, /music/ etc). The alternative (I think) is to load any files for the menu specifically and not fill the dictionaries until a server is chosen.

@cyberjunk
Copy link
Owner

For point 1), I left Init() as-is in case any of the resources are used for creating the menu screen, since it happens before picking a server it'll use the resources in the parent directory (/rooms/, /music/ etc). The alternative (I think) is to load any files for the menu specifically and not fill the dictionaries until a server is chosen.

Hmm, right. I didn't thought about the login-screen which is in fact legacy resource stuff. :-/

@skittles1
Copy link
Contributor Author

Not sure either how to handle the file lookups... first thing that springs to mind is something like git where each server's resources would effectively be a branch with some changed files, but seems a bit overkill for this (and I wouldn't even know where to start). I'm tempted to just rename all the rooms and anything else that gets changed but it'll get messy fast with 3 or more servers active.

@skittles1
Copy link
Contributor Author

The other option (for rooms at least) is to have only subdirectories for each server in the /rooms/ directory, and have a full set of rooms in each. I don't know if this should be done for roomtextures/sounds/music as it'd be raising the size of the client a fair bit for identical resources.

Or we could host separate Ogre clients for 103 and 105/112 as both codebases are modifying different rooms now and the currently hosted Ogre compatibility with 105/112 will break if 103 updates (assuming the 103 version of the rooms goes into the patcher).

@cyberjunk
Copy link
Owner

Or we could host separate Ogre clients for 103 and 105/112 ...

I think this might be the cleanest solution after all...

I do see the advantage of sharing resources between installations in the approach here, and may be a shared config/key-binding would also be useful.

On the other hand, if you think this through, it might very well happen at some point these servers also require (or want) slightly modified binaries. This would make a dedicated client-installation a requirement and would make this approach here obsolete again and just leaves us with the disadvantages - so may be we should just setup dedicated patcher entries?

After all the 600mb of the client isn't that much compared to other games, I guess you can easily have 3-4 of these installations on your hard drive..

@skittles1
Copy link
Contributor Author

Yeah I agree with that, though I'm wondering what you think we should do with different copies of the same room in the resources repository. Could keep what's there now as the canonical "standard room set" and each server takes care of moving their own rooms in when they build/test?

@cyberjunk
Copy link
Owner

Yeah I agree with that, though I'm wondering what you think we should do with different copies of the same room in the resources repository. Could keep what's there now as the canonical "standard room set" and each server takes care of moving their own rooms in when they build/test?

I would prefer having all the different resource sets for the different servers in the resources-repository and then making the build-process select a set. That would be a lot easier to test for anyone including me.

For example the /resources/rooms/ folder should probably be empty and then get filled by the build-process (e.g. VS post-build command) with the content of a server-specific rooms folder.

It's basically a bit like your current approach here, but instead of making the client select the resource set, make the build-process do it.

The VANILLA preprocessor flag could may be already be used to select the 101 set?
Not sure how to handle the others, probably a good idea to start introducing preprocessor flags for 103, 105 and 112 which so far are all combined as !VANILLA.

@skittles1
Copy link
Contributor Author

Ok I can try and do that when I get a bit of time free, you can probably close this 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.

2 participants