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

Initial implementation of an osu!lazer-based diffcalc server #2

Merged
merged 20 commits into from Oct 8, 2018

Conversation

Projects
None yet
2 participants
@smoogipoo
Contributor

smoogipoo commented Jun 3, 2018

Prereqs:

Currently reads .osus named by their beatmap id from the beatmaps/ directory.

@@ -0,0 +1,72 @@
// Copyright (c) 2007-2018 ppy Pty Ltd <contact@ppy.sh>.
// Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE

This comment has been minimized.

@peppy

peppy Jun 3, 2018

Member

linking to wrong repo

=> CommandLineApplication.Execute<Program>(args);
// Todo: Move to configuration
private const string connection_string = "Server=localhost;Database=osu;User ID=root;SslMode=None;Pooling=true; Min pool size = 50; Max pool size = 200; Charset=utf8;";

This comment has been minimized.

@peppy

peppy Jun 3, 2018

Member

Definitely should be read from configuration file (check out appsettings.json documentation and implementation in https://github.com/ppy/osu-server/pull/1/files#diff-79f3b5ed3196a6a5ec6c0257b17fbbfa).

We may want to move this implementation to a shared project.

This comment has been minimized.

@smoogipoo

smoogipoo Jun 4, 2018

Contributor

Yeah not sure if a shared project is super necessary here, it seems like the only code that's really duplicated is:

var env = Environment.GetEnvironmentVariable("APP_ENV") ?? "development";
var config = new ConfigurationBuilder()
    .SetBasePath(Directory.GetCurrentDirectory())
    .AddJsonFile("appsettings.json", optional: true, reloadOnChange: false)
    .AddJsonFile($"appsettings.{env}.json", optional: true, reloadOnChange: false)
    .AddEnvironmentVariables()
    .Build();

Soo...

This comment has been minimized.

@peppy

peppy Jun 4, 2018

Member

Yeah, maybe not. I think @notbakaneko is planning on looking into whether there's an abstraction on top of appsettings to provide typing of settings to avoid string lookups.

This comment has been minimized.

@smoogipoo

smoogipoo Jun 4, 2018

Contributor

TOO LATE. We'll revise on this at a later point /shrug!

private Database database;

This comment has been minimized.

@peppy

peppy Jun 3, 2018

Member

extra newline

var tasks = new List<Task>();
using (var reader = database.RunQuery("SELECT osu_beatmaps.beatmap_id, checksum FROM osu_beatmaps ORDER BY beatmap_id ASC"))

This comment has been minimized.

@peppy

peppy Jun 3, 2018

Member

osu_beatmaps. prefix not required

while (reader.Read())
{
int id = reader.GetInt32(0);
string hash = reader.IsDBNull(1) ? string.Empty : reader.GetString(1);

This comment has been minimized.

@peppy

peppy Jun 3, 2018

Member

not being used?

var attributes = new Dictionary<string, object>();
double starRating = ruleset.CreateDifficultyCalculator(playable, toModArray(mod)).Calculate(attributes);
database.RunNonQuery(

This comment has been minimized.

@peppy

peppy Jun 3, 2018

Member

Database connections are not thread-safe like this. you will need to create a new connection per task. Connection pooling is handled automatically (based on connection string) so there's no harm in using (new MysqlConnection()) each time.

This comment has been minimized.

@smoogipoo

smoogipoo Jun 4, 2018

Contributor

A new connection is still created with every query (see usages of getConnection() in Database.cs.

var parameters = new List<MySqlParameter>
{
new MySqlParameter("@beatmap_id", beatmapId),

This comment has been minimized.

@peppy

peppy Jun 3, 2018

Member

The newer helper methods in Mysql.Data don't require explicit MySqlParameter construction for each parameter.

This comment has been minimized.

@smoogipoo

smoogipoo Jun 4, 2018

Contributor

Which helper methods are you referring to, exactly?

This comment has been minimized.

var attributes = new Dictionary<string, object>();
double starRating = ruleset.CreateDifficultyCalculator(playable, toModArray(mod)).Calculate(attributes);
using (var conn = database.GetConnection())

This comment has been minimized.

@peppy

peppy Jun 4, 2018

Member

best to move connection retrieval outside foreach

@peppy peppy merged commit 36867f2 into ppy:master Oct 8, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment