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

improved database persistence for biota_properties_generator #2616

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gmriggs
Copy link
Collaborator

@gmriggs gmriggs commented Jan 14, 2020

I did a search through all the weenie_properties_generators, and for every object except 31986, the probabilities are always in ascending order.

However, this is not 100% foolproof, and there are a few caveats with this: some generators contain spans with the same probabilities, usually a bunch of -1's... these will still be sorted into a non-guaranteed order

This should cover 99.9%+ of the scenarios though, and let us get away with restoring the biota_properties_generator in the original weenie_properties_generator ordering, without an explicit 'order' column

@gmriggs gmriggs requested a review from Mag-nus January 14, 2020 19:05
@Mag-nus
Copy link
Member

Mag-nus commented Jan 15, 2020

Should something similar be applied to the WorldDatabase where the weenies are read?

@gmriggs
Copy link
Collaborator Author

gmriggs commented Jan 15, 2020

Should something similar be applied to the WorldDatabase where the weenies are read?

Technically when a bunch of the of the weenie* tables are read in, it should be ordering by id, ensuring the records are returned in the order they were entered into the db.

The World DB does not have the same problems as the Shard DB though. The Shard DB biota tables are written incorrectly, as a result of EF writing collections / sets in its own internal way, providing no interface to specify or guarantee the ordering.

So if the question is 'should we also be sorting the weenie generators table by probability when it is read in', the answer is no, most definitely not. In fact, we shouldn't really even be doing it for the shard database -- it is only a hack to avoid having to place the order column on the biota_generators table. The order column would be the 'real' way to do it.

@@ -223,7 +223,7 @@ public static Biota GetBiota(ShardDbContext context, uint id)
if (populatedCollectionFlags.HasFlag(PopulatedCollectionFlags.BiotaPropertiesEnchantmentRegistry)) biota.BiotaPropertiesEnchantmentRegistry = context.BiotaPropertiesEnchantmentRegistry.Where(r => r.ObjectId == biota.Id).ToList();
if (populatedCollectionFlags.HasFlag(PopulatedCollectionFlags.BiotaPropertiesEventFilter)) biota.BiotaPropertiesEventFilter = context.BiotaPropertiesEventFilter.Where(r => r.ObjectId == biota.Id).ToList();
if (populatedCollectionFlags.HasFlag(PopulatedCollectionFlags.BiotaPropertiesFloat)) biota.BiotaPropertiesFloat = context.BiotaPropertiesFloat.Where(r => r.ObjectId == biota.Id).ToList();
if (populatedCollectionFlags.HasFlag(PopulatedCollectionFlags.BiotaPropertiesGenerator)) biota.BiotaPropertiesGenerator = context.BiotaPropertiesGenerator.Where(r => r.ObjectId == biota.Id).ToList();
if (populatedCollectionFlags.HasFlag(PopulatedCollectionFlags.BiotaPropertiesGenerator)) biota.BiotaPropertiesGenerator = context.BiotaPropertiesGenerator.Where(r => r.ObjectId == biota.Id).OrderBy(r => r.Probability).ToList();
Copy link
Member

Choose a reason for hiding this comment

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

Your reasoning makes sense, and this change should work.

I'd like to see a comment above this line though. It should describe why we're doing the OrderBy here. It would also act as an indicator to note that this line is different among all the other queries.

In the future, if we do add an order column to this table, we could get caught out by not realizing this band-aid exists.

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.

None yet

3 participants