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

core: use ColorMixin for colors #604

Merged
merged 2 commits into from Mar 27, 2022
Merged

core: use ColorMixin for colors #604

merged 2 commits into from Mar 27, 2022

Conversation

Rainrider
Copy link
Member

Closes #590

No BC breaks. I'll switch elements and stuff if you want this. Just left them in for now to demonstrate BC.

@Rainrider Rainrider added the core label Mar 27, 2022
@Rainrider Rainrider self-assigned this Mar 27, 2022
p3lim
p3lim previously approved these changes Mar 27, 2022
Copy link
Member

@p3lim p3lim left a comment

Choose a reason for hiding this comment

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

LGTM

@p3lim
Copy link
Member

p3lim commented Mar 27, 2022

If you want to update everything else to use :GetRGB() and whatnot that's up to you, but it'll eventually be something we'll want to do.

This probably also involves updating the color-related APIs oUF exposes, but they can probably be left alone unless there are performance reasons to update them.

@Rainrider
Copy link
Member Author

Used following code for a perf benchmark where color is a local copy of Interface/SharedXML/Color.lua and Interface/SharedXML/Mixin.lua (only what is needed for the call to CreateFromMixins):

package.path = package.path .. ';./?.lua'

require('color')

local function benchmark(times, name, f, ...)
	local start = os.clock()
	for i = 1, times do
		f(...)
	end
	local elapsed = (os.clock() - start)

	print(('%16s: %9.3f ms'):format(name, elapsed * 1e4))
end

local r, g, b = 0.1, 0.4, 0.8
local color = CreateColor(r, g, b)
color[1] = r
color[2] = g
color[3] = b
color.atlas = 'some-atlas'

local color2 = { r, g, b, atlas = 'some-atlas' }

local function callRGB(color)
	local r, g, b = color:GetRGB()
end

local function callUnpack(color)
	local r, g, b = unpack(color)
end

local function getByIndex(color)
	local r, g, b = color[1], color[2], color[3]
end

local function getByName(color)
	local r, g, b = color.r, color.g, color.b
end

benchmark(1e6, 'color:getRGB()', callRGB, color)
benchmark(1e6, 'unpack(mixin)', callUnpack, color)
benchmark(1e6, 'index mixin', getByIndex, color)
benchmark(1e6, 'name mixin', getByName, color)
benchmark(1e6, 'unpack(simple)', callUnpack, color2)
benchmark(1e6, 'index simple', getByIndex, color2)
benchmark(1e6, 'name simple', getByName, color2)

Results:

  color:getRGB():   795.010 ms
   unpack(mixin):  1178.900 ms
     index mixin:   591.340 ms
      name mixin:   532.440 ms
  unpack(simple):   970.390 ms
    index simple:   689.990 ms
     name simple:   684.070 ms

So I kept the direct table access instead of calling color:GetRGB() but retrieve the color components by name rather than index. This allows us to ditch the array part in the next expac.

@Rainrider Rainrider requested a review from p3lim March 27, 2022 19:49
This is in preparation to ditch the array part we added to
ColorMixin for backwards compatibility.
I keep the direct tble access instead of calling color:getRGB()
for perfomance.
Copy link
Member

@p3lim p3lim left a comment

Choose a reason for hiding this comment

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

👍

@Rainrider Rainrider merged commit da7554a into master Mar 27, 2022
@Rainrider Rainrider deleted the color-mixin branch March 27, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ColorMixin
2 participants