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

Remove cast to Game in AudioSystem #472

Merged
merged 3 commits into from Jul 2, 2019
Merged

Conversation

@Hyperpred
Copy link
Contributor

Hyperpred commented Jun 16, 2019

PR Details

Added GameSettings to the services and then removed the cast to Game in AudioSystem. This is beginning the work of decoupling the Systems from Game to allow for a Headless Game to be created.

Description

As above, just added GameSettings to the Services so AudioSystem (and other classes as needed) can access them without casting to Game.

In AudioSystem specifically, the only reference to Game is to get the settings. Other areas such as SyncScript would still be able to access the settings directly.

I did not add GameSettings to IGame because of it being unavailable to that project. Another possibility would be to call Content.Load in Audio system and then unload after the settings have been used.

Related Issue

None - if needed I can create an issue as I go about decoupling systems if this is even a path that we want to take.

Motivation and Context

Decoupling the classes will help allow for a Headless game which will be needed for using the engine as a dedicated server. Specifically, in this case, the only coupling is to load the settings and this was the most straight forward way to break that coupling.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jun 16, 2019

CLA assistant check
All committers have signed the CLA.

{
var settings = ((Game)Game)?.Settings?.Configurations?.Get<AudioEngineSettings>();
{
var settings = Services.GetService<GameSettings>()?.Configurations?.Get<AudioEngineSettings>();

This comment has been minimized.

Copy link
@xen2

xen2 Jun 26, 2019

Member

What about adding GameSettings Settings { get; } to IGame and GameBase (as abstract)? GameSettings doesn't really sound like something that should be a service (maybe a IGameSettingsProvider, but simply adding it to IGame sounds simpler and good enough for now).

This comment has been minimized.

Copy link
@Hyperpred

Hyperpred Jun 26, 2019

Author Contributor

Adding GameSeetings to IGame was my first intention. However, GameSettings is in Xenko.Engine and IGame is in Xenko.Game. I think that it might be worthwhile to just move GameSettings to Xenko.Game but I don't know what the side effects of that would be.

If IGameSettingsProvider only has a GameSettings {get;} I don't know if that kind of abstraction would make sense or not. However, I'm open to do whichever you prefer.

This comment has been minimized.

Copy link
@xen2

xen2 Jun 27, 2019

Member

Xenko.Engine references Xenko.Games and AudioSystem (which is a GameSystemBase) already has a member Game.
So simply adding IGame.Settings member should be enough to be able to do Game.Settings within AudioSystem class.

This comment has been minimized.

Copy link
@Hyperpred

Hyperpred Jun 27, 2019

Author Contributor

image
The problem is that Xenko.Games does not reference Xenko.Engine. So right now GameSettings is in Xenko.Engine and IGame is in Xenko.Games. That is why I was unable to add GameSettings to IGame. Another alternative if we don't move GameSettings might be to call Content.Load in AudioSystem?

This comment has been minimized.

Copy link
@xen2

xen2 Jun 27, 2019

Member

My bad, sorry for the misunderstanding!
In that case, let's go for a IGameSettingsService with a GameSettings { get; } member.
The Game class could implement this interface directly and register it in the list of services (so no need to create a new class implementing it).

This comment has been minimized.

Copy link
@Hyperpred

Hyperpred Jun 29, 2019

Author Contributor

I've made the requested update. I wasn't sure if you wanted me to squash the commits or just add the commit on top of the first merge request.

If you need me to do anything different please let me know. But, I have added the IGameSettingsService and it is implemented by Game now.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jul 2, 2019

@Hyperpred I think you have a problem with your CRLF settings.
Various files are comitted with CRLF but they should only have LF.
git config should have core.autocrlf=true. I suspect you have it set to false or input.

I will fix it for you on this PR and merge.

@xen2 xen2 merged commit 0a8d7ba into xenko3d:master Jul 2, 2019
2 checks passed
2 checks passed
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
phr00t added a commit to phr00t/FocusEngine that referenced this pull request Jul 2, 2019
@Hyperpred

This comment has been minimized.

Copy link
Contributor Author

Hyperpred commented Jul 2, 2019

@xen2 You are correct, I didn't have that setting so it was defaulted to false. I have added it to my Xenko config.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jul 2, 2019

@Hyperpred just so you know, it's usually recommended to have it set to true globally on Windows and most project follow that convention since it's the default for Git for Windows.
That way it won't happen again if you clone a separate xenko or another repo later.

@Kryptos-FR

This comment has been minimized.

Copy link
Collaborator

Kryptos-FR commented Jul 3, 2019

@xen2 It is even better to set it on the .gitattributes file. I will open a pull-request.

# Set the default behavior, in case people don't have core.autocrlf set.
* text=auto

# Explicitly declare text files you want to always be normalized and converted
# to native line endings on checkout.
*.c       text
*.cpp     text
*.cs      text
*.h       text

# Declare files that will always have CRLF line endings on checkout.
*.bat     text eol=crlf
*.csproj  text eol=crlf
*.sln     text eol=crlf

# Declare files that will always have LF line endings on checkout.
*.sh      text eol=lf
@Kryptos-FR Kryptos-FR mentioned this pull request Jul 3, 2019
1 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.