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

[Proto Optimization] Optomizing the proto file #151

Open
11 of 21 tasks
Tracked by #64
klingbolt opened this issue Jul 31, 2023 · 1 comment
Open
11 of 21 tasks
Tracked by #64

[Proto Optimization] Optomizing the proto file #151

klingbolt opened this issue Jul 31, 2023 · 1 comment

Comments

@klingbolt
Copy link
Contributor

klingbolt commented Jul 31, 2023

The current .proto file was written based on the way data is stored in the XML files, but we aren't restricted to that. The following is a list of improvements that can be made to either improve data storage in the protobuf formatted files or improve the performance of the Godot code. Many on this list will be changing the code so that a default value is always stored as 0 if possible and that both the XML Converter and Burrito handle the default value correctly. This list includes the current default values if applicable.

The following fields could be improved if the default value is stored in the proto file as 0 where both Burrito and xml_converter know that "0" should be interpreted as the default value. For now, these values are not implemented in burrito, so we are going to comment them out in the proto. They can be re-enabled once a solution is found to keep track of default values in xml_converter.

  • Min and Max
    • Size (Default = 5 for min and 2048 for max)
    • Fade (Defaults = 700 for near and 900 for far)
  • Set values to 0 if equal to default
    • Scale (Default = 1)
    • Icon Size (Default = 1)
    • Animation Speed (change from default= 1 to default = 0)
    • Height Offset (Default = 1.5)
    • Map Display Size (Default = 20)
    • Trigger Range (default = 2)
@AsherGlick
Copy link
Owner

AsherGlick commented Oct 10, 2023

Some brainstorming:

  • Category.name - Is not used in the proto now that icons and trails are nested

    • Category.display_name should be used to infer what name should be when writing to xml without name otherwise set
      • Category.display_name should also be renamed to Category.name
  • Icon.texture_path, Trail.texture_path - is possibly a highly duplicated string, existing on every icon and trail that uses the same texture

    • Replace with uint32 index into a top-level array of texture path objects, useful if there is extra info that should be included alongside the texture
    • Becomes a uint32 index into an array of generic strings
  • Icon.rgba_color Trail.rgba_color should have "white" as the default value

    • Not allow pure black as a color by making #00000000 and #FFFFFFFF equivalent. We already have precedent for not allowing all 0's in the filter messages. Where if they are all left as false then no filters are applied.
    • Inverted color value (#FFFFFFFF - color) which would mean #FFFFFFFF becomes #00000000 and visa versa.
    • Reverse #FFFFFFFF and #00000000 specifically so they represent each other. This adds some hidden complexity. Probably the worst option so far.
  • Icon.can_fade Trail.can_fade - should default to false

    • Rename to prevent_fading and add inversion logic in converter
    • Rename to always_visible and add inversion logic in converter
    • Rename to render_over_player and add inversion logic in converter
  • Category.default_visibility - should default to false

    • Rename to hidden_by_default and add inversion logic in converter
    • Rename to hide_by_default and add inversion logic in converter
  • Icon.distance_fade_start Trail.distance_fade_start - should default to 0, currently the default is 700.

    • Change to a fixed distance offset of the default value
    • Change to a default-zero scaling percentage modifier using something like the algorithm
    if N < 0:
      default / POW((10/9), -N/100)
    else
      default * POW((10/9), N/100)
    
    • 0 is a special value of default as fade at 0 does not make sense
  • Icon.distance_fade_end Trail.distance_fade_end - should default to 0, currently the default is 900.

    • Change to distance_fade_duration and use as a distance from distance_fade_start (still would have a default problem of 200)
    • Change to distance_fade_length and use as a distance from distance_fade_start (still would have a default problem of 200)
    • 0 is a special value of default as fade at 0 does not make sense
  • Icon.height_offset - Should default to 0, currently the default is 1.5

    • Remove this in favor of combining it with the vertical position
  • Icon.tentative__scale should become Icon.scale and default to zero

    • Icon.scale Trail.scale - should default to 0, currently default to 1
      • Use the POW((10/9), -N/100) algorithm above
      • Let 0 be an invalid value and any 0 or missing values become 1 as scale to 0 does not make sense
  • Icon.map_display_size, Trail.map_display_size - default is 20

    • Make this some sort of scale
    • Use the image's default size as the default
    • 0 is a special value of default as display at 0 pixels does not make sense
  • Icon.scale_on_map_with_zoom should default false, currently true

    • Rename to constant_size_on_map and add inversion logic in converter
    • Rename to ignore_map_zoom_scale and add inversion logic in converter
  • Icon.minimum_size_on_screen - Should default to 0, currently the default is 5.

    • Maybe change to be a delta or scale of the images original size
    • Maybe can be set by a user setting as well
    • zero is a special value of "default"
  • Icon.maximum_size_on_screen - Should default to 0, currently the default is 2048. Maybe change to be a delta or scale of the images original size.

    • Maybe change to be a delta or scale of the images original size
    • Maybe can be set by a user setting as well
    • zero is a special value of "default"
  • Trigger.range - should default to 0, currently defaults to 2 ?

    • ?
  • Icon.category, Trail.category - no longer needed in the proto because everything is hierarchical

    • This value should be removed and Category.names of the parents should be used to infer the value when parsing the proto

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

No branches or pull requests

2 participants