From 54b25ef08e7f1dd96363cda43303b69d0e32f8c6 Mon Sep 17 00:00:00 2001 From: Emi Matchu Date: Sun, 10 Nov 2024 11:36:23 -0800 Subject: [PATCH] Reintroduce some of our modeling refactors, without touching items Okay so, when we reverted a buncha stuff in e3d196f, it was in response to a bug where item modeling data was getting deleted. And I was tired, and just took a big simple hammer to it of reverting all the modeling refactors. Here, we reintroduce *some* of them: the biology ones before the item bug. And tests still pass, and in fact I can un-pending some of them! I might also try to reapply the change where we extract it all into a new file, but without the item parts. ```shell git cherry-pick --no-commit 13ceec8fcc866fcc0a81e86a74b6e79b5f7d4f6c git cherry-pick --no-commit f81415d3270e7bfee17101475c8b98b79e32fde1 git cherry-pick --no-commit c03e7446e3d5e1323414039555d11a76abce2efe git cherry-pick --no-commit 52ca41dbff4c08165012afa774677c938bdfcaf3 ``` --- app/models/alt_style.rb | 9 --- app/models/pet.rb | 163 +++++++++++++++++++++++++--------------- app/models/pet_state.rb | 66 +--------------- app/models/pet_type.rb | 13 ++-- spec/models/pet_spec.rb | 12 +-- 5 files changed, 116 insertions(+), 147 deletions(-) diff --git a/app/models/alt_style.rb b/app/models/alt_style.rb index 0e9a47bf..b822a2dd 100644 --- a/app/models/alt_style.rb +++ b/app/models/alt_style.rb @@ -74,15 +74,6 @@ class AltStyle < ApplicationRecord Item.appearances_for(items, self, ...) end - def biology=(biology) - # TODO: This is very similar to what `PetState` does, but like… much much - # more compact? Idk if I'm missing something, or if I was just that much - # more clueless back when I wrote it, lol 😅 - self.swf_assets = biology.values.map do |asset_data| - SwfAsset.from_biology_data(self.body_id, asset_data) - end - end - # At time of writing, most batches of Alt Styles thumbnails used a simple # pattern for the item thumbnail URL, but that's not always the case anymore. # For now, let's keep using this format as the default value when creating a diff --git a/app/models/pet.rb b/app/models/pet.rb index 3765bec9..189794bf 100644 --- a/app/models/pet.rb +++ b/app/models/pet.rb @@ -4,66 +4,15 @@ class Pet < ApplicationRecord attr_reader :items, :pet_state, :alt_style def load!(timeout: nil) - viewer_data = Neopets::CustomPets.fetch_viewer_data(name, timeout:) - use_viewer_data(viewer_data) + viewer_data_hash = Neopets::CustomPets.fetch_viewer_data(name, timeout:) + use_viewer_data(ViewerData.new(viewer_data_hash)) end def use_viewer_data(viewer_data) - pet_data = viewer_data[:custom_pet] - - raise UnexpectedDataFormat unless pet_data[:species_id] - raise UnexpectedDataFormat unless pet_data[:color_id] - raise UnexpectedDataFormat unless pet_data[:body_id] - - has_alt_style = pet_data[:alt_style].present? - - self.pet_type = PetType.find_or_initialize_by( - species_id: pet_data[:species_id].to_i, - color_id: pet_data[:color_id].to_i - ) - - begin - new_image_hash = Neopets::CustomPets.fetch_image_hash(self.name) - rescue => error - Rails.logger.warn "Failed to load image hash: #{error.full_message}" - end - self.pet_type.image_hash = new_image_hash if new_image_hash.present? - - # With an alt style, `body_id` in the biology data refers to the body ID of - # the *alt* style, not the usual pet type. (We have `original_biology` for - # *some* of the pet type's situation, but not it's body ID!) - # - # So, in the alt style case, don't update `body_id` - but if this is our - # first time seeing this pet type and it doesn't *have* a `body_id` yet, - # let's not be creating it without one. We'll need to model it without the - # alt style first. (I don't bother with a clear error message though 😅) - self.pet_type.body_id = pet_data[:body_id] unless has_alt_style - if self.pet_type.body_id.nil? - raise UnexpectedDataFormat, - "can't process alt style on first occurrence of pet type" - end - - pet_state_biology = has_alt_style ? pet_data[:original_biology] : - pet_data[:biology_by_zone] - raise UnexpectedDataFormat if pet_state_biology.empty? - pet_state_biology[0] = nil # remove effects if present - @pet_state = self.pet_type.add_pet_state_from_biology! pet_state_biology - - if has_alt_style - raise UnexpectedDataFormat unless pet_data[:alt_color] - raise UnexpectedDataFormat if pet_data[:biology_by_zone].empty? - - @alt_style = AltStyle.find_or_initialize_by(id: pet_data[:alt_style].to_i) - @alt_style.assign_attributes( - color_id: pet_data[:alt_color].to_i, - species_id: pet_data[:species_id].to_i, - body_id: pet_data[:body_id].to_i, - biology: pet_data[:biology_by_zone], - ) - end - - @items = Item.collection_from_pet_type_and_registries(self.pet_type, - viewer_data[:object_info_registry], viewer_data[:object_asset_registry]) + self.pet_type = viewer_data.pet_type + @pet_state = viewer_data.pet_state + @alt_style = viewer_data.alt_style + @items = viewer_data.items end def wardrobe_query @@ -89,11 +38,8 @@ class Pet < ApplicationRecord before_validation do pet_type.save! - if @pet_state - @pet_state.save! - @pet_state.handle_assets! - end - + @pet_state.save! if @pet_state + if @items @items.each do |item| item.save! if item.changed? @@ -113,5 +59,98 @@ class Pet < ApplicationRecord end class UnexpectedDataFormat < RuntimeError;end + + # A representation of a Neopets::CustomPets viewer data response, translated + # to DTI's database models! + class ViewerData + def initialize(viewer_data_hash) + @custom_pet = viewer_data_hash[:custom_pet] + @object_info_registry = viewer_data_hash[:object_info_registry] + @object_asset_registry = viewer_data_hash[:object_asset_registry] + end + + def pet_type + @pet_type ||= begin + raise UnexpectedDataFormat unless @custom_pet[:species_id] + raise UnexpectedDataFormat unless @custom_pet[:color_id] + raise UnexpectedDataFormat unless @custom_pet[:body_id] + + @custom_pet => {species_id:, color_id:} + PetType.find_or_initialize_by(species_id:, color_id:).tap do |pet_type| + # Apply the pet's body ID to the pet type, unless it's wearing an alt + # style, in which case ignore it, because it's the *alt style*'s body ID. + # (This can theoretically cause a problem saving a new pet type when + # there's an alt style too!) + pet_type.body_id = @custom_pet[:body_id] unless @custom_pet[:alt_style] + if pet_type.body_id.nil? + raise UnexpectedDataFormat, + "can't process alt style on first occurrence of pet type" + end + + # Try using this pet for the pet type's thumbnail, but don't worry + # if it fails. + begin + pet_type.consider_pet_image(@custom_pet[:name]) + rescue => error + Rails.logger.warn "Failed to load pet image: #{error.full_message}" + end + end + end + end + + def pet_state + @pet_state ||= begin + swf_asset_ids = biology_assets.map(&:remote_id).sort.join(",") + pet_type.pet_states.find_or_initialize_by(swf_asset_ids:).tap do |pet_state| + pet_state.swf_assets = biology_assets + end + end + end + + def alt_style + @alt_style ||= begin + return nil unless @custom_pet[:alt_style] + raise UnexpectedDataFormat unless @custom_pet[:alt_color] + + id = @custom_pet[:alt_style].to_i + AltStyle.find_or_initialize_by(id:).tap do |alt_style| + alt_style.assign_attributes( + color_id: @custom_pet[:alt_color].to_i, + species_id: @custom_pet[:species_id].to_i, + body_id: @custom_pet[:body_id].to_i, + swf_assets: alt_style_assets, + ) + end + end + end + + def items + @items ||= Item.collection_from_pet_type_and_registries( + pet_type, @object_info_registry, @object_asset_registry + ) + end + + private + + def biology_assets + @biology_assets ||= begin + biology = @custom_pet[:alt_style].present? ? + @custom_pet[:original_biology] : + @custom_pet[:biology_by_zone] + assets_from_biology(biology) + end + end + + def alt_style_assets + raise UnexpectedDataFormat if @custom_pet[:biology_by_zone].empty? + assets_from_biology(@custom_pet[:biology_by_zone]) + end + + def assets_from_biology(biology) + raise UnexpectedDataFormat if biology.empty? + body_id = @custom_pet[:body_id].to_i + biology.values.map { |b| SwfAsset.from_biology_data(body_id, b) } + end + end end diff --git a/app/models/pet_state.rb b/app/models/pet_state.rb index 61671777..b59f9397 100644 --- a/app/models/pet_state.rb +++ b/app/models/pet_state.rb @@ -6,8 +6,7 @@ class PetState < ApplicationRecord has_many :contributions, :as => :contributed, :inverse_of => :contributed # in case of duplicates being merged has_many :outfits - has_many :parent_swf_asset_relationships, :as => :parent, - :autosave => false + has_many :parent_swf_asset_relationships, :as => :parent has_many :swf_assets, :through => :parent_swf_asset_relationships belongs_to :pet_type @@ -15,8 +14,6 @@ class PetState < ApplicationRecord delegate :species_id, :species, :color_id, :color, to: :pet_type alias_method :swf_asset_ids_from_association, :swf_asset_ids - - attr_writer :parent_swf_asset_relationships_to_update # A simple ordering that tries to bring reliable pet states to the front. scope :emotion_order, -> { @@ -111,71 +108,10 @@ class PetState < ApplicationRecord self['swf_asset_ids'] = ids end - def handle_assets! - @parent_swf_asset_relationships_to_update.each do |rel| - rel.swf_asset.save! - rel.save! - end - end - def to_param "#{id}-#{pose.split('_').map(&:capitalize).join('-')}" end - def self.from_pet_type_and_biology_info(pet_type, info) - swf_asset_ids = [] - info.each do |zone_id, asset_info| - if zone_id.present? && asset_info - swf_asset_ids << asset_info[:part_id].to_i - end - end - swf_asset_ids_str = swf_asset_ids.sort.join(',') - if pet_type.new_record? - pet_state = self.new :swf_asset_ids => swf_asset_ids_str - else - pet_state = self.find_or_initialize_by( - pet_type_id: pet_type.id, - swf_asset_ids: swf_asset_ids_str - ) - end - existing_swf_assets = SwfAsset.biology_assets.includes(:zone). - where(remote_id: swf_asset_ids) - existing_swf_assets_by_id = {} - existing_swf_assets.each do |swf_asset| - existing_swf_assets_by_id[swf_asset.remote_id] = swf_asset - end - existing_relationships_by_swf_asset_id = {} - unless pet_state.new_record? - pet_state.parent_swf_asset_relationships.each do |relationship| - existing_relationships_by_swf_asset_id[relationship.swf_asset_id] = relationship - end - end - pet_state.pet_type = pet_type # save the second case from having to look it up by ID - relationships = [] - info.each do |zone_id, asset_info| - if zone_id.present? && asset_info - swf_asset_id = asset_info[:part_id].to_i - swf_asset = existing_swf_assets_by_id[swf_asset_id] - unless swf_asset - swf_asset = SwfAsset.new - swf_asset.remote_id = swf_asset_id - end - swf_asset.origin_biology_data = asset_info - swf_asset.origin_pet_type = pet_type - relationship = existing_relationships_by_swf_asset_id[swf_asset.id] - unless relationship - relationship ||= ParentSwfAssetRelationship.new - relationship.parent = pet_state - relationship.swf_asset_id = swf_asset.id - end - relationship.swf_asset = swf_asset - relationships << relationship - end - end - pet_state.parent_swf_asset_relationships_to_update = relationships - pet_state - end - private # A helper for the `pose=` method. diff --git a/app/models/pet_type.rb b/app/models/pet_type.rb index 055cca2d..a73f510d 100644 --- a/app/models/pet_type.rb +++ b/app/models/pet_type.rb @@ -57,6 +57,14 @@ class PetType < ApplicationRecord basic_image_hash || self['image_hash'] || 'deadbeef' end + def consider_pet_image(pet_name) + # If we already have a basic image hash, don't worry about it! + return if basic_image_hash? + + # Otherwise, use this as the new image hash for this pet type. + self.image_hash = Neopets::CustomPets.fetch_image_hash(pet_name) + end + def possibly_new_color self.color || Color.new(id: self.color_id) end @@ -71,11 +79,6 @@ class PetType < ApplicationRecord species_human_name: possibly_new_species.human_name) end - def add_pet_state_from_biology!(biology) - pet_state = PetState.from_pet_type_and_biology_info(self, biology) - pet_state - end - def canonical_pet_state # For consistency (randomness is always scary!), we use the PetType ID to # determine which gender to prefer, if it's not built into the color. That diff --git a/spec/models/pet_spec.rb b/spec/models/pet_spec.rb index a6eb874e..8e9474be 100644 --- a/spec/models/pet_spec.rb +++ b/spec/models/pet_spec.rb @@ -36,7 +36,7 @@ RSpec.describe Pet, type: :model do let(:asset_ids) { biology_assets.map(&:remote_id) } they("are all new") { should all be_new_record } - pending("match the expected IDs (before saving)") do + they("match the expected IDs (before saving)") do expect(asset_ids).to contain_exactly(10083, 11613, 14187, 14189) end they("match the expected IDs (after saving)") do @@ -44,7 +44,7 @@ RSpec.describe Pet, type: :model do expect(asset_ids).to contain_exactly(10083, 11613, 14187, 14189) end they("are saved when saving the pet") { pet.save!; should all be_persisted } - pending("have the expected asset metadata (before saving)") do + they("have the expected asset metadata (before saving)") do should contain_exactly( a_record_matching( type: "biology", @@ -187,11 +187,11 @@ RSpec.describe Pet, type: :model do biology_assets.select(&:new_record?).map(&:remote_id) } - pending("are partially new, partially existing") do + they("are partially new, partially existing") do expect(persisted_asset_ids).to contain_exactly(10083, 11613) expect(new_asset_ids).to contain_exactly(10448, 10451) end - pending("match the expected IDs (before saving)") do + they("match the expected IDs (before saving)") do expect(asset_ids).to contain_exactly(10083, 11613, 10448, 10451) end they("match the expected IDs (after saving)") do @@ -199,7 +199,7 @@ RSpec.describe Pet, type: :model do expect(asset_ids).to contain_exactly(10083, 11613, 10448, 10451) end they("are saved when saving the pet") { new_pet.save!; should all be_persisted } - pending("have the expected asset metadata (before saving)") do + they("have the expected asset metadata (before saving)") do should contain_exactly( a_record_matching( type: "biology", @@ -289,7 +289,7 @@ RSpec.describe Pet, type: :model do let(:asset_ids) { biology_assets.map(&:remote_id) } they("are all new") { should all be_new_record } - pending("match the expected IDs (before saving)") do + they("match the expected IDs (before saving)") do expect(asset_ids).to contain_exactly(331, 332, 333, 23760, 23411) end they("match the expected IDs (after saving)") do