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

Fix offset being ignored by inventory bar HUD #2094

Closed
wants to merge 1 commit into from

Conversation

rubenwardy
Copy link
Member

minetest.register_on_joinplayer(function(player)
    minetest.chat_send_player(player:get_player_name(), "Hello!")
    minetest.after(2, function()
        player:hud_add({
            hud_elem_type = "inventory",
            text = "main",
            number = 3,
            item = 1,
            direction = 2,
            position = {x=0, y=0.5},
            offset = {x=300, y = 100}
        })
        player:hud_add({
            hud_elem_type = "inventory",
            text = "main",
            number = 3,
            item = 2,
            direction = 0,
            position = {x=0, y=0.5}
        })
    end)
end)

@rubenwardy rubenwardy force-pushed the hud_offset branch 2 times, most recently from 4e1fba1 to 027e404 Compare January 9, 2015 17:19
@rubenwardy rubenwardy changed the title Fix offset being ignored by inventory bar Fix offset being ignored by inventory bar HUD Jan 9, 2015
@ShadowNinja ShadowNinja added @ Client / Audiovisuals Unconfirmed bug Bug report that has not been confirmed to exist/be reproducible labels Jan 10, 2015
@rubenwardy
Copy link
Member Author

Just look at the code, the offset is never used.

{
#ifdef HAVE_TOUCHSCREENGUI
if ( (g_touchscreengui) && (offset == 0))
if ( (g_touchscreengui) && (inv_offset == 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the superfluous parenthesis?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just changed the existing code, but I'll fix that.

@rubenwardy
Copy link
Member Author

@sapier, please may you check that the scaling works on this?

@KenneyNL
Copy link

KenneyNL commented Jun 2, 2015

What's the status on this? Just tried it in the latest version and offset still does absolutely nothing.

@rubenwardy
Copy link
Member Author

I was waiting for @sapier to test the scaling, as I'm unsure whether it works correctly.

}

offset.X *= g_settings->getFloat("hud_scaling") *
Copy link
Contributor

Choose a reason for hiding this comment

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

Requesting the setting? Twice? Best, it should be cached somewhere, retrieving settings with the hashmap isn't fast at all.

@rubenwardy rubenwardy force-pushed the hud_offset branch 5 times, most recently from ee4bc83 to 9b5faa7 Compare June 24, 2015 16:40
@rubenwardy
Copy link
Member Author

Fixed, rebased and ready for review.

@rubenwardy
Copy link
Member Author

Bump, is it worth rebasing this?

@rubenwardy
Copy link
Member Author

Rebased and ready for review

@paramat
Copy link
Contributor

paramat commented Feb 24, 2016

Yes it's worth rebasing, thanks, sorry it was neglected.

@paramat paramat added Bug Issues that were confirmed to be a bug and removed Unconfirmed bug Bug report that has not been confirmed to exist/be reproducible labels Mar 18, 2016
@rubenwardy
Copy link
Member Author

Rebased and code style fixed
Added minimal test example to start post.
Whilst rebase, I noticed a bug: #3970

@paramat paramat added this to the 0.4.14 milestone Apr 7, 2016
@paramat
Copy link
Contributor

paramat commented Apr 7, 2016

I assume the bug is fixed by this PR?

@rubenwardy
Copy link
Member Author

That rendering bug is present both before and after my PR. Currently working out the cause.

@sofar
Copy link
Contributor

sofar commented Apr 7, 2016

Read the code change, and this is almost trivial. The actual effective change is maybe 3 lines, the rest are all cleanups or passing the parameter around the function calls.

@rubenwardy
Copy link
Member Author

Graphics bug moved to here: #3970

@sfan5
Copy link
Member

sfan5 commented Apr 9, 2016

👍

@paramat paramat modified the milestone: 0.4.14 Apr 9, 2016
@paramat
Copy link
Contributor

paramat commented Apr 10, 2016

Code reviewed @rubenwardy is this tested? if so +1.

@rubenwardy
Copy link
Member Author

I've tested this, and it makes the attached code example work.

@paramat
Copy link
Contributor

paramat commented Apr 10, 2016

👍

void Hud::drawItems(v2s32 upperleftpos, s32 itemcount, s32 offset,
InventoryList *mainlist, u16 selectitem, u16 direction)
void Hud::drawItems(v2s32 upperleftpos, s32 itemcount, s32 inv_offset,
InventoryList *mainlist, u16 selectitem, u16 direction, const v2s32 &offset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is offset a const ref while upperleftpos isn't? Please make both a const ref, or pass offset by value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this makes that much sense any way... sizeof(v2s32) == 8 so on 64 bit architectures the pointer isn't smaller than the actual data. This means an extra level of indirection, which is bad, and for (almost) no use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I'm just going to change this while merging...

@kwolekr
Copy link
Contributor

kwolekr commented Apr 11, 2016

👍

@kwolekr
Copy link
Contributor

kwolekr commented Apr 11, 2016

eae3395

@kwolekr kwolekr closed this Apr 11, 2016
@rubenwardy rubenwardy deleted the hud_offset branch December 12, 2016 13:32
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.

None yet

9 participants